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

[Kunkun Jiang]

Hi Oliver,

On 2024/6/21 6:25, Oliver Upton wrote:
> 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 at 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.
This makes sense. I will modify it in the next version.

> static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> {
>         struct its_collection *collection;
>         struct kvm *kvm = its->dev->kvm;
>         u32 target_addr, coll_id;
>         u64 val;
>         int ret;
>
>         BUG_ON(esz > sizeof(val));
>         ret = kvm_read_guest_lock(kvm, gpa, &val, esz);
It should also be modified synchronously, right?
>>   	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.
Sorry, there is a problem here. I will pay attention to it in the future.
Thank you for your correction.

Thanks,
Kunkun Jiang