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

[Kunkun Jiang]

Hi Marc，

Thank you very much for reviewing.

On 2024/6/13 22: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.
The latest patch has been modified. See link below.
>> ---
>>   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
The latest patch has been modified. See link below.
> 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.
A patch has been added to check it.
Link: 
https://lore.kernel.org/kvmarm/20240620130650.1279-1-jiangkunkun@huawei.com/

Thanks,
Kunkun Jiang