Re: [PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller <ricarkol@google.com>]

On Tue, May 03, 2022 at 07:40:46PM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 4/27/22 20:48, Ricardo Koller wrote:
> > Restoring a corrupted collection entry is being ignored and treated as
> maybe precise what is a corrupted ITE (out of range id or not matching
> guest RAM)
> > success. More specifically, vgic_its_restore_cte failure is treated as
> > success by vgic_its_restore_collection_table.  vgic_its_restore_cte uses
> > a positive number to return ITS error codes, and +1 to return success.
> Not fully correct as vgic_its_restore_cte() also returns a bunch of
> generic negative error codes. vgic_its_alloc_collection() only returns
> one positive ITS error code.

Thanks, will clarify this. I was just focusing on that positive ITS
error code being treated as success by the caller.

> > The caller then uses "ret > 0" to check for success. An additional issue
> > is that invalid entries return 0 and although that doesn't fail the
> > restore, it leads to skipping all the next entries.
> Isn't what we want. If I remember correctly an invalid entry corresponds
> to the end of the collection table, hence the break.
> see vgic_its_save_collection_table() and "add a last dummy element with
> valid bit unset".

Ah, definitely. This was incorrect then.

> >
> > Fix this by having vgic_its_restore_cte return negative numbers on
> > error, and 0 on success (which includes skipping an invalid entry).
> > While doing that, also fix alloc_collection return codes to not mix ITS
> > error codes (positive numbers) and generic error codes (negative
> > numbers).
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++----------
> >  1 file changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index fb2d26a73880..86c26aaa8275 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -999,15 +999,16 @@ static bool vgic_its_check_event_id(struct vgic_its *its, struct its_device *dev
> >  	return __is_visible_gfn_locked(its, gpa);
> >  }
> >  
> > +/*
> > + * Adds a new collection into the ITS collection table.
> nit: s/Adds/Add here and below
> > + * Returns 0 on success, and a negative error value for generic errors.
> > + */
> >  static int vgic_its_alloc_collection(struct vgic_its *its,
> >  				     struct its_collection **colp,
> >  				     u32 coll_id)
> >  {
> >  	struct its_collection *collection;
> >  
> > -	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_ACCOUNT);
> >  	if (!collection)
> >  		return -ENOMEM;
> > @@ -1101,7 +1102,12 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  
> >  	collection = find_collection(its, coll_id);
> >  	if (!collection) {
> > -		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > +		int ret;
> > +
> > +		if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> > +			return E_ITS_MAPC_COLLECTION_OOR;
> > +
> > +		ret = vgic_its_alloc_collection(its, &collection, coll_id);
> >  		if (ret)
> >  			return ret;
> >  		new_coll = collection;
> > @@ -1256,6 +1262,10 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
> >  		if (!collection) {
> >  			int ret;
> >  
> > +			if (!vgic_its_check_id(its, its->baser_coll_table,
> > +						coll_id, NULL))
> > +				return E_ITS_MAPC_COLLECTION_OOR;
> > +
> >  			ret = vgic_its_alloc_collection(its, &collection,
> >  							coll_id);
> >  			if (ret)
> > @@ -2497,6 +2507,10 @@ static int vgic_its_save_cte(struct vgic_its *its,
> >  	return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
> >  }
> >  
> > +/*
> > + * Restores a collection entry into the ITS collection table.
> > + * Returns 0 on success, and a negative error value for generic errors.
> > + */
> >  static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> >  {
> >  	struct its_collection *collection;
> > @@ -2511,7 +2525,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> >  		return ret;
> >  	val = le64_to_cpu(val);
> >  	if (!(val & KVM_ITS_CTE_VALID_MASK))
> > -		return 0;
> > +		return 0; /* invalid entry, skip it */
> >  
> >  	target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
> >  	coll_id = val & KVM_ITS_CTE_ICID_MASK;
> > @@ -2523,11 +2537,15 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> >  	collection = find_collection(its, coll_id);
> >  	if (collection)
> >  		return -EEXIST;
> > +
> > +	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> > +		return -EINVAL;
> > +
> >  	ret = vgic_its_alloc_collection(its, &collection, coll_id);
> >  	if (ret)
> >  		return ret;
> >  	collection->target_addr = target_addr;
> > -	return 1;
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -2593,15 +2611,12 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
> >  
> >  	while (read < max_size) {
> >  		ret = vgic_its_restore_cte(its, gpa, cte_esz);
> > -		if (ret <= 0)
> > +		if (ret < 0)
> >  			break;
> >  		gpa += cte_esz;
> >  		read += cte_esz;
> >  	}
> >  
> > -	if (ret > 0)
> > -		return 0;
> > -
> >  	return ret;
> >  }
> >  
> Thanks
> 
> Eric
> 

Thanks,
Ricardo
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm