Re: [PATCH v2 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller <ricarkol@google.com>]

On Tue, May 03, 2022 at 07:14:24PM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 4/27/22 20:48, Ricardo Koller wrote:
> > Try to improve the predictability of ITS save/restores by failing
> > commands that would lead to failed saves. More specifically, fail any
> > command that adds an entry into an ITS table that is not in guest
> > memory, which would otherwise lead to a failed ITS save ioctl. There
> > are already checks for collection and device entries, but not for
> > ITEs.  Add the corresponding check for the ITT when adding ITEs.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 51 ++++++++++++++++++++++++----------
> >  1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 2e13402be3bd..e14790750958 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -894,6 +894,18 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
> >  	return update_affinity(ite->irq, vcpu);
> >  }
> >  
> > +static bool __is_visible_gfn_locked(struct vgic_its *its, gpa_t gpa)
> > +{
> > +	gfn_t gfn = gpa >> PAGE_SHIFT;
> > +	int idx;
> > +	bool ret;
> > +
> > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > +	return ret;
> > +}
> > +
> >  /*
> >   * 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
> > @@ -908,9 +920,7 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> >  	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
> >  	phys_addr_t base = GITS_BASER_ADDR_48_to_52(baser);
> >  	int esz = GITS_BASER_ENTRY_SIZE(baser);
> > -	int index, idx;
> > -	gfn_t gfn;
> > -	bool ret;
> > +	int index;
> >  
> >  	switch (type) {
> >  	case GITS_BASER_TYPE_DEVICE:
> > @@ -933,12 +943,11 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> >  			return false;
> >  
> >  		addr = base + id * esz;
> > -		gfn = addr >> PAGE_SHIFT;
> >  
> >  		if (eaddr)
> >  			*eaddr = addr;
> >  
> > -		goto out;
> > +		return __is_visible_gfn_locked(its, addr);
> >  	}
> >  
> >  	/* calculate and check the index into the 1st level */
> > @@ -964,16 +973,30 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> >  	/* Find the address of the actual entry */
> >  	index = id % (SZ_64K / esz);
> >  	indirect_ptr += index * esz;
> > -	gfn = indirect_ptr >> PAGE_SHIFT;
> >  
> >  	if (eaddr)
> >  		*eaddr = indirect_ptr;
> >  
> > -out:
> > -	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > -	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > -	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > -	return ret;
> > +	return __is_visible_gfn_locked(its, indirect_ptr);
> > +}
> > +
> > +/*
> > + * Check whether an event ID can be stored in the corresponding Interrupt
> > + * Translation Table, which starts at device->itt_addr.
> > + */
> > +static bool vgic_its_check_event_id(struct vgic_its *its, struct its_device *device,
> > +		u32 event_id)
> > +{
> > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > +	int ite_esz = abi->ite_esz;
> > +	gpa_t gpa;
> > +
> > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > +		return false;
> > +
> > +	gpa = device->itt_addr + event_id * ite_esz;
> > +	return __is_visible_gfn_locked(its, gpa);
> >  }
> >  
> >  static int vgic_its_alloc_collection(struct vgic_its *its,
> > @@ -1061,9 +1084,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  	if (!device)
> >  		return E_ITS_MAPTI_UNMAPPED_DEVICE;
> >  
> > -	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > -		return E_ITS_MAPTI_ID_OOR;
> I would put
>     if (!vgic_its_check_event_id(its, device, event_id))
>         return E_ITS_MAPTI_ID_OOR;
> here instead of after since if the evend_id not correct, no use to look
> the ite for instance.

Thanks Eric. Will fix in v2.

> > -
> >  	if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
> >  		lpi_nr = its_cmd_get_physical_id(its_cmd);
> >  	else
> > @@ -1076,6 +1096,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  	if (find_ite(its, device_id, event_id))
> >  		return 0;
> >  
> > +	if (!vgic_its_check_event_id(its, device, event_id))
> > +		return E_ITS_MAPTI_ID_OOR;
> > +
> >  	collection = find_collection(its, coll_id);
> >  	if (!collection) {
> >  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> Besides look good to me
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm