[PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Eric Auger]

Hi,

On 6/13/24 16:44, Marc Zyngier wrote:
> On Thu, 13 Jun 2024 10:38:11 +0100,
> Kunkun Jiang <jiangkunkun at huawei.com> wrote:
>>
>> 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 at huawei.com>
> 
>   ^^^^^^^^^^^^^ This isn't a valid tag!
> 
>> Signed-off-by: Kunkun Jiang <jiangkunkun at huawei.com>
> 
> Something is wrong. Either Shusen Li is the author and you are merely
> posting it, in which case there should be a 'From' line in the commit
> message, or you are co-authors and there should be a 'Co-developed-by'
> right after Shusen Li's SoB.
> 
> Please read Documentation/process/submitting-patches.rst for the
> details.
> 
>> ---
>>  arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
>> index 40bb43f20bf3..dd5fba1e8ba3 100644
>> --- a/arch/arm64/kvm/vgic/vgic-its.c
>> +++ b/arch/arm64/kvm/vgic/vgic-its.c
>> @@ -1140,8 +1140,12 @@ 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;
>> +	const struct vgic_its_abi *abi;
>> +	int dte_esz;
>> +	u64 val;
> 
> Aside from 'gpa', all the extra variables should be moved into the
> !valid block. And it would be nicer if this variable was called
> something more descriptive.
> 
>>  
>> -	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 +1165,13 @@ 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) {
>> +		abi = vgic_its_get_abi(its);
>> +		dte_esz = abi->dte_esz;
>> +		val = cpu_to_le64(0);
>> +		vgic_write_guest_lock(kvm, gpa, &val, dte_esz);
> 
> This is just a bit wrong:
> 
> - you store 0 in a u64 after having (pointlessly) converted it to le64
> 
> - you write 'dte_esz' bytes into guest memory, but you don't have any
>   check whether this is *larger* than the u64 you have passed
> 
> Similar issue exists in all the vgic_its_save_*() functions, calling
> for a generic fix, as we're just lucky to never have needed a new ABI
> so far. At the very least a check that we deal with data that is
> exactly 8 bytes.
> 
> Also, is the device table the only one that is subject to this
> 'reload' problem? How about the Collection table?
The collection table is not indexed by the collection ID and when saving
we end up writing an invalid dummy entry to close the recording. So I
don't think we have the same problem there.

For ITT I think we have the same issue on restore because the restore
scans the table until it gets a first valid entry and then chain on next
item. If we have stale entries this looks like the same issue.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
>