Re: [PATCH 1/3] KVM: arm64: vgic-its: Add a data length check in vgic_its_save_*

[Oliver Upton <oliver.upton@linux.dev>]

Hi,

On Thu, Jun 20, 2024 at 09:06:48PM +0800, Kunkun Jiang wrote:
> In all the vgic_its_save_*() functinos, it does not check
> whether the data length is larger than 8 bytes before
> calling vgic_write_guest_lock. This patch add the check.
> 
> Link: https://lore.kernel.org/kvmarm/86v82ckimh.wl-maz@kernel.org/
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 40bb43f20bf3..060605fba3b6 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -2094,6 +2094,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>  	       ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |
>  		ite->collection->collection_id;
>  	val = cpu_to_le64(val);
> +	BUG_ON(ite_esz > sizeof(val));

Does it really make sense to blow up the kernel over this? (hint: no)

What _might_ make sense if if you bugged the VM and failed the ioctl,
i.e.

	if (KVM_BUG_ON(ite_esz != sizeof(val), kvm))
		return -EINVAL;

Also, this isn't even asserting the right thing. You want to assert that
the u64 being written to memory is *exactly* the size of a single ITE.
No more, no less.

>  	return vgic_write_guest_lock(kvm, gpa, &val, ite_esz);
>  }
>  
> @@ -2246,6 +2247,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
>  	       (itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) |
>  		(dev->num_eventid_bits - 1));
>  	val = cpu_to_le64(val);
> +	BUG_ON(dte_esz > sizeof(dte_esz));

Did you even test this? A bit of substitution arrives at:

	BUG_ON(8 > sizeof(unsigned int));

See the issue?

Please do not test these sort of untested patches on the list, it is a
waste of everyone's time.

-- 
Thanks,
Oliver