Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Auger Eric]

Hi,

On 13/10/2017 15:16, Christoffer Dall wrote:
> On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
>> At the moment the device table save() returns -EINVAL if
>> vgic_its_check_id() fails to return the gpa of the entry
>> associated to the device/collection id. Let vgic_its_check_id()
>> return an int instead of a bool and return a more precised
>> error value:
>> - EINVAL in case the id is out of range
>> - EFAULT if the gpa is not provisionned or is not valid
>>
> 
> This is just to ease debugging, yes?

I understood user-space should be able to discriminate between bad guest
programming and values corrupted by the userspace (regs for instance).
In first case QEMU should not abort. In latter case it should abort.

In vgic_its_check_id we are checking the L1 entry validity bit and in
case it is invalid we can't compute the GPA of the entry. I was thinking
we should return -EFAULT in that case. But maybe returning EFAULT in
case the BASER<n> address is not reachable also is wrong because that
may be caused by the userspace writing a wrong value. Sigh ...

Thanks

Eric
> 
> Seems a bit invasive to me for that purpose.
> 
>> We also check first the GITS_BASER<n> Valid bit is set.
>>
>> This allows the userspace to discriminate failure reasons.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>>
>> need to CC stable
> 
> Is this solving a real issue that someone has?  Otherwise I don't think
> it should be cc'ed to stable, nor am I sure it warrants merging as a
> fix.
> 
> Thanks,
> -Christoffer
> 
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 36 insertions(+), 17 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 76bed2d..c1f7972 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>>  }
>>  
>>  /*
>> - * Check whether an ID can be stored into the corresponding guest table.
>> + * vgic_its_check_id - Check whether an ID can be stored into
>> + * the corresponding guest table.
>> + *
>>   * For a direct table this is pretty easy, but gets a bit nasty for
>>   * indirect tables. We check whether the resulting guest physical address
>>   * is actually valid (covered by a memslot and guest accessible).
>>   * For this we have to read the respective first level entry.
>> + *
>> + * @its: its handle
>> + * @baser: GITS_BASER<n> register
>> + * @id: id of the device/collection
>> + * @eaddr: output gpa of the corresponding table entry
>> + *
>> + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
>> + * the address cannot be computed or is not valid
>>   */
>> -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>> -			      gpa_t *eaddr)
>> +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>> +			     gpa_t *eaddr)
>>  {
>>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>>  	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
>> @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>>  	int index;
>>  	gfn_t gfn;
>>  
>> +	if (!(baser & GITS_BASER_VALID))
>> +		return -EFAULT;
>> +
>>  	switch (type) {
>>  	case GITS_BASER_TYPE_DEVICE:
>>  		if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
>> -			return false;
>> +			return -EINVAL;
>>  		break;
>>  	case GITS_BASER_TYPE_COLLECTION:
>>  		/* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
>>  		if (id >= BIT_ULL(16))
>> -			return false;
>> +			return -EINVAL;
>>  		break;
>>  	default:
>> -		return false;
>> +		return -EINVAL;
>>  	}
>>  
>>  	if (!(baser & GITS_BASER_INDIRECT)) {
>>  		phys_addr_t addr;
>>  
>>  		if (id >= (l1_tbl_size / esz))
>> -			return false;
>> +			return -EINVAL;
>>  
>>  		addr = BASER_ADDRESS(baser) + id * esz;
>>  		gfn = addr >> PAGE_SHIFT;
>>  
>>  		if (eaddr)
>>  			*eaddr = addr;
>> -		return kvm_is_visible_gfn(its->dev->kvm, gfn);
>> +		if (kvm_is_visible_gfn(its->dev->kvm, gfn))
>> +			return 0;
>> +		else
>> +			return -EFAULT;
>>  	}
>>  
>>  	/* calculate and check the index into the 1st level */
>>  	index = id / (SZ_64K / esz);
>>  	if (index >= (l1_tbl_size / sizeof(u64)))
>> -		return false;
>> +		return -EINVAL;
>>  
>>  	/* Each 1st level entry is represented by a 64-bit value. */
>>  	if (kvm_read_guest(its->dev->kvm,
>>  			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
>>  			   &indirect_ptr, sizeof(indirect_ptr)))
>> -		return false;
>> +		return -EFAULT;
>>  
>>  	indirect_ptr = le64_to_cpu(indirect_ptr);
>>  
>>  	/* check the valid bit of the first level entry */
>>  	if (!(indirect_ptr & BIT_ULL(63)))
>> -		return false;
>> +		return -EFAULT;
>>  
>>  	/*
>>  	 * Mask the guest physical address and calculate the frame number.
>> @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>>  
>>  	if (eaddr)
>>  		*eaddr = indirect_ptr;
>> -	return kvm_is_visible_gfn(its->dev->kvm, gfn);
>> +	if (kvm_is_visible_gfn(its->dev->kvm, gfn))
>> +		return 0;
>> +	else
>> +		return -EFAULT;
>>  }
>>  
>>  static int vgic_its_alloc_collection(struct vgic_its *its,
>> @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
>>  {
>>  	struct its_collection *collection;
>>  
>> -	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
>> +	if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
>>  		return E_ITS_MAPC_COLLECTION_OOR;
>>  
>>  	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
>> @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>  	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>>  	struct its_device *device;
>>  
>> -	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, NULL))
>>  		return E_ITS_MAPD_DEVICE_OOR;
>>  
>>  	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
>> @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>>  		int ret;
>>  		gpa_t eaddr;
>>  
>> -		if (!vgic_its_check_id(its, baser,
>> -				       dev->device_id, &eaddr))
>> -			return -EINVAL;
>> +		ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
>> +		if (ret)
>> +			return ret;
>>  
>>  		ret = vgic_its_save_itt(its, dev);
>>  		if (ret)
>> -- 
>> 2.5.5
>>