Re: [PATCH v3] xen/gnttab: fix gnttab_acquire_resource()

[Juergen Gross <jgross@xxxxxxxx>]

On 09.09.22 15:12, Andrew Cooper wrote:
> On 09/09/2022 13:53, Juergen Gross wrote:
> > Commit 9dc46386d89d ("gnttab: work around "may be used uninitialized"
> > warning") was wrong, as vaddrs can legitimately be NULL in case
> > XENMEM_resource_grant_table_id_status was specified for a grant table
> > v1. This would result in crashes in debug builds due to
> > ASSERT_UNREACHABLE() triggering.
> >
> > Check vaddrs only to be NULL in the rc == 0 case.
> >
> > Expand the tests in tools/tests/resource to verify that using
> > XENMEM_resource_grant_table_id_status on a V1 grant table will result
> > in EINVAL.
> >
> > Fixes: 9dc46386d89d ("gnttab: work around "may be used uninitialized" warning")
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> # xen
> > Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>
> > ---
> > V2:
> > - rework (Jan Beulich, Julien Grall)
> > V3:
> > - added test support (Andrew Cooper)
> > ---
> >   tools/tests/resource/test-resource.c | 11 +++++++++++
> >   xen/common/grant_table.c             |  2 +-
> >   2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/tests/resource/test-resource.c 
> > b/tools/tests/resource/test-resource.c
> > index 189353ebcb..71a81f207e 100644
> > --- a/tools/tests/resource/test-resource.c
> > +++ b/tools/tests/resource/test-resource.c
> > @@ -106,6 +106,17 @@ static void test_gnttab(uint32_t domid, unsigned int 
> > nr_frames,
> >       if ( rc )
> >           return fail("    Fail: Unmap grant table %d - %s\n",
> >                       errno, strerror(errno));
> > +
> > +    /* Verify that the attempt to map the status frames is failing for V1. */
> > +    res = xenforeignmemory_map_resource(
> > +        fh, domid, XENMEM_resource_grant_table,
> > +        XENMEM_resource_grant_table_id_status, 0, 1,
> > +        (void **)&gnttab, PROT_READ | PROT_WRITE, 0);
> > +    if ( res || errno != EINVAL )
> > +        fail("    Fail: Map status not failing with EINVAL %d - %s\n",
> > +             res ? 0 : errno, res ? "no error" : strerror(errno));
> I'd recommend not checking for EINVAL specifically.  This has bitten XTF
> in the past when XSAs have caused one error to turn into another, and
> plenty of hypercalls have exact errnos which change depending on how Xen
> is compiled.
>
> I'd go with the more simple:
>
> if ( res )
> {
>      fail("    Fail: Managed to map gnttab v2 status frames in v1 mode\n");
>      xenforeignmemory_unmap_resource(fh, res);
> }
>
> Everything else looks fine, so I'm happy to fix this up on commit.
Thanks. Please adapt the commit message accordingly.

BTW, I've verified that the system crashes without the hypervisor change.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature