| Message ID | 20220909125347.25734-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xen/gnttab: fix gnttab_acquire_resource() | expand |
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@suse.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> # xen > Release-acked-by: Henry Wang <Henry.Wang@arm.com> > --- > 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. ~Andrew
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@suse.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> # xen >> Release-acked-by: Henry Wang <Henry.Wang@arm.com> >> --- >> 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
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)); + if ( res ) + xenforeignmemory_unmap_resource(fh, res); } static void test_domain_configurations(void) diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index ad773a6996..fba329dcc2 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4140,7 +4140,7 @@ int gnttab_acquire_resource( * on non-error paths, and hence it needs setting to NULL at the top of the * function. Leave some runtime safety. */ - if ( !vaddrs ) + if ( !rc && !vaddrs ) { ASSERT_UNREACHABLE(); rc = -ENODATA;
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@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> # xen Release-acked-by: Henry Wang <Henry.Wang@arm.com> --- 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(-)