Re: [RFC PATCH v1 21/25] hw/xen: Add emulated implementation of grant table operations

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Tue, 2023-03-07 at 16:07 +0000, Paul Durrant wrote:
> On 02/03/2023 15:34, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > 
> > This is limited to mapping a single grant at a time, because under Xen the
> > pages are mapped *contiguously* into qemu's address space, and that's very
> > hard to do when those pages actually come from anonymous mappings in qemu
> > in the first place.
> > 
> > Eventually perhaps we can look at using shared mappings of actual objects
> > for system RAM, and then we can make new mappings of the same backing
> > store (be it deleted files, shmem, whatever). But for now let's stick to
> > a page at a time.
> > 
> > Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > ---
> >   hw/i386/kvm/xen_gnttab.c | 299 ++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 296 insertions(+), 3 deletions(-)
> > 
> [snip]
> > +static uint64_t gnt_ref(XenGnttabState *s, grant_ref_t ref, int prot)
> > +{
> > +    uint16_t mask = GTF_type_mask | GTF_sub_page;
> > +    grant_entry_v1_t gnt, *gnt_p;
> > +    int retries = 0;
> > +
> > +    if (ref >= s->max_frames * ENTRIES_PER_FRAME_V1 ||
> > +        s->map_track[ref] == UINT8_MAX) {
> > +        return INVALID_GPA;
> > +    }
> > +
> > +    if (prot & PROT_WRITE) {
> > +        mask |= GTF_readonly;
> > +    }
> > +
> > +    gnt_p = &s->entries.v1[ref];
> > +
> > +    /*
> > +     * The guest can legitimately be changing the GTF_readonly flag. Allow
> 
> I'd call a guest playing with the ref after setting GTF_permit_access a 
> buggy guest and not bother with the loop.

Didn't we have this argument already when I tried to get you to change
your one? :)

I argue that it's OK for a guest to *increase* permissions to include
write permission, even while a read-only grant may be in progress.

And also to *decrease* permissions to take away write permission, even
while read-only grants may be in progress.

The loop is what Xen does, so let's do it that way.

> 
> > +     * that, but don't let a malicious guest cause a livelock.
> > +     */
> > +    for (retries = 0; retries < 5; retries++) {
> > +        uint16_t new_flags;
> > +
> > +        /* Read the entry before an atomic operation on its flags */
> > +        gnt = *(volatile grant_entry_v1_t *)gnt_p;
> > +
> > +        if ((gnt.flags & mask) != GTF_permit_access ||
> > +            gnt.domid != DOMID_QEMU) {
> > +            return INVALID_GPA;
> > +        }
> > +
> > +        new_flags = gnt.flags | GTF_reading;
> > +        if (prot & PROT_WRITE) {
> > +            new_flags |= GTF_writing;
> > +        }
> > +
> > +        if (qatomic_cmpxchg(&gnt_p->flags, gnt.flags, new_flags) == 
> > gnt.flags) {
> 
> Xen actually does a cmpxchg on both the flags and the domid. We probably 
> ought to fail to set the flags if the guest is playing with the domid
> but since we're single-tenant it doesn't *really* matter... just a 
> nice-to-have. So...

Yeah, changing the *domid* while it's active is definitely not an OK
thing to do.

Attachment: smime.p7s
Description: S/MIME cryptographic signature