[PATCH 0/3] KVM: arm64: vgic-its: Some fixes about vgic-its

[PATCH 0/3] KVM: arm64: vgic-its: Some fixes about vgic-its

[Kunkun Jiang]

There are three fixes about vgic-its:
Patch 1: A data length check is added in vgic_its_save_*
Patch 2-3: Fix two bugs in the scenario of continuous save and restore

Kunkun Jiang (3):
  KVM: arm64: vgic-its: Add a data length check in vgic_its_save_*
  KVM: arm64: vgic-its: Clear dte when mapd unmaps a device
  KVM: arm64: vgic-its: Clear ite when discard frees an ite

 arch/arm64/kvm/vgic/vgic-its.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

-- 
2.33.0

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

[Kunkun Jiang]

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));
 	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));
 	return vgic_write_guest_lock(kvm, ptr, &val, dte_esz);
 }
 
@@ -2433,6 +2435,7 @@ static int vgic_its_save_cte(struct vgic_its *its,
 	       ((u64)collection->target_addr << KVM_ITS_CTE_RDBASE_SHIFT) |
 	       collection->collection_id);
 	val = cpu_to_le64(val);
+	BUG_ON(esz > sizeof(val));
 	return vgic_write_guest_lock(its->dev->kvm, gpa, &val, esz);
 }
 
-- 
2.33.0

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

[Oliver Upton]

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

Re: [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@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

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

[Oliver Upton]

On Fri, Jun 21, 2024 at 09:43:05AM +0800, Kunkun Jiang wrote:
> > 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?

Do you mean replacing the other BUG_ON()'s in the ITS code with the
pattern I'd recommended earlier?

That'd be great if you could do that.

> > >   	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.

Apologies for being firm on this, but outside of RFCs the expectation is
that the author test changes before posting to the list.

In that spirit, a reproducer for the issue you observe in KVM selftests
would be great. We have some minimal support for dealing with an ITS
over there now.

-- 
Thanks,
Oliver

[PATCH 2/3] KVM: arm64: vgic-its: Clear dte when mapd unmaps a device

[Kunkun Jiang]

vgic_its_save_device_tables will traverse its->device_list to
save dte for each device. vgic_its_restore_device_tables will
traverse each entry of device table and check if it is valid.
Restore if valid.

But when mapd unmaps a devices, we do not invalidate the
corresponding dte. In the scenario of continuous save
and restore, there may be a situation where a device's dte
is not saved but is restored. This is unreasonable and may
cause restore to fail. This patch clears the corresponding
dte when mapd unmaps a device.

Singed-off-by: Shusen Li <lishusen2@huawei.com>
Co-developed-by: Shusen Li <lishusen2@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 060605fba3b6..8e11859ff803 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -1140,8 +1140,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	u8 num_eventid_bits = its_cmd_get_size(its_cmd);
 	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
 	struct its_device *device;
+	gpa_t gpa;
 
-	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
+	if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
 		return E_ITS_MAPD_DEVICE_OOR;
 
 	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
@@ -1161,8 +1162,14 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	 * The spec does not say whether unmapping a not-mapped device
 	 * is an error, so we are done in any case.
 	 */
-	if (!valid)
+	if (!valid) {
+		int dte_esz = vgic_its_get_abi(its)->dte_esz;
+		u64 val = 0;
+
+		BUG_ON(dte_esz > sizeof(val));
+		vgic_write_guest_lock(kvm, gpa, &val, dte_esz);
 		return 0;
+	}
 
 	device = vgic_its_alloc_device(its, device_id, itt_addr,
 				       num_eventid_bits);
-- 
2.33.0

[PATCH 3/3] KVM: arm64: vgic-its: Clear ite when discard frees an ite

[Kunkun Jiang]

When discard frees an ite, we do not invalidate the
corresponding ite. In the scenario of continuous save and
restore, there may be a situation where an ite is not saved
but is restored. This is unreasonable and may cause restore
to fail. This patch clears the corresponding ite when discard
frees a ite.

Link: https://lore.kernel.org/kvmarm/8f9d74fc-f9d9-43ac-a387-91ff804cfaf1@redhat.com/
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 8e11859ff803..a10516ff760a 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -782,6 +782,10 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
 
 	ite = find_ite(its, device_id, event_id);
 	if (ite && its_is_collection_mapped(ite->collection)) {
+		struct its_device *device = find_its_device(its, device_id);
+		int ite_esz = vgic_its_get_abi(its)->ite_esz;
+		gpa_t gpa = device->itt_addr + ite->event_id * ite_esz;
+		u64 val = 0;
 		/*
 		 * Though the spec talks about removing the pending state, we
 		 * don't bother here since we clear the ITTE anyway and the
@@ -790,6 +794,9 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
 		vgic_its_invalidate_cache(its);
 
 		its_free_ite(kvm, ite);
+
+		BUG_ON(ite_esz > sizeof(val));
+		vgic_write_guest_lock(kvm, gpa, &val, ite_esz);
 		return 0;
 	}
 
-- 
2.33.0