Re: [PATCH v2 2/4] KVM: arm64: vgic: Add more checks when restoring ITS tables

[Ricardo Koller <ricarkol@google.com>]

On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 4/27/22 20:48, Ricardo Koller wrote:
> > Try to improve the predictability of ITS save/restores (and debuggability
> > of failed ITS saves) by failing early on restore when trying to read
> > corrupted tables.
> >
> > Restoring the ITS tables does some checks for corrupted tables, but not as
> > many as in a save: an overflowing device ID will be detected on save but
> > not on restore.  The consequence is that restoring a corrupted table won't
> > be detected until the next save; including the ITS not working as expected
> > after the restore.  As an example, if the guest sets tables overlapping
> > each other, which would most likely result in some corrupted table, this is
> > what we would see from the host point of view:
> >
> > 	guest sets base addresses that overlap each other
> > 	save ioctl
> > 	restore ioctl
> > 	save ioctl (fails)
> >
> > Ideally, we would like the first save to fail, but overlapping tables could
> > actually be intended by the guest. So, let's at least fail on the restore
> > with some checks: like checking that device and event IDs don't overflow
> > their tables.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index e14790750958..fb2d26a73880 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -2198,6 +2198,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> >  	if (!collection)
> >  		return -EINVAL;
> >  
> > +	if (find_ite(its, dev->device_id, event_id))
> > +		return -EINVAL;
> Unsure about that. Nothing in the arm-vgic-its.rst doc says that the
> KVM_DEV_ARM_ITS_RESTORE_TABLES ioctl cannot be called several times
> (although obviously useless)

In that case, maybe we could ignore the new repeated entry? or
overwrite the old one?  find_ite() only returns the first (device_id,
event_id) match. So, it's like the new one is ignored already.  The
arm arm says this about MAPI commands in this situation:

    If there is an existing mapping for the EventID-DeviceID
    combination, behavior is UNPREDICTABLE.

And, just in case, the main reason for adding this check was to avoid
failing the next ITS save. The idea is to try to fail as soon as
possible, not in possibly many days during the next migration attempt.

> > +
> > +	if (!vgic_its_check_event_id(its, dev, event_id))
> > +		return -EINVAL;
> > +
> >  	ite = vgic_its_alloc_ite(dev, collection, event_id);
> >  	if (IS_ERR(ite))
> >  		return PTR_ERR(ite);
> > @@ -2319,6 +2325,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> >  				void *ptr, void *opaque)
> >  {
> >  	struct its_device *dev;
> > +	u64 baser = its->baser_device_table;
> >  	gpa_t itt_addr;
> >  	u8 num_eventid_bits;
> >  	u64 entry = *(u64 *)ptr;
> > @@ -2339,6 +2346,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> >  	/* dte entry is valid */
> >  	offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT;
> >  
> > +	if (find_its_device(its, id))
> > +		return -EINVAL;
> same here.
> > +
> > +	if (!vgic_its_check_id(its, baser, id, NULL))
> > +		return -EINVAL;
> > +
> >  	dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits);
> >  	if (IS_ERR(dev))
> >  		return PTR_ERR(dev);
> Thanks
> 
> Eric
>

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