xen/common/domctl.c | 2 ++ 1 file changed, 2 insertions(+)
A hypervisor built without CONFIG_GDBSX will crash in case the
XEN_DOMCTL_gdbsx_guestmemio domctl is being called, as the call will
end up in iommu_do_domctl() with d == NULL:
(XEN) CPU: 6
(XEN) RIP: e008:[<ffff82d040269984>] iommu_do_domctl+0x4/0x30
(XEN) RFLAGS: 0000000000010202 CONTEXT: hypervisor (d0v0)
(XEN) rax: 00000000000003e8 rbx: ffff830856277ef8 rcx: ffff830856277fff
...
(XEN) Xen call trace:
(XEN) [<ffff82d040269984>] R iommu_do_domctl+0x4/0x30
(XEN) [<ffff82d04035cd5f>] S arch_do_domctl+0x7f/0x2330
(XEN) [<ffff82d040239e46>] S do_domctl+0xe56/0x1930
(XEN) [<ffff82d040238ff0>] S do_domctl+0/0x1930
(XEN) [<ffff82d0402f8c59>] S pv_hypercall+0x99/0x110
(XEN) [<ffff82d0402f5161>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x11/0x90
(XEN) [<ffff82d040366288>] S lstar_enter+0x128/0x130
(XEN)
(XEN) Pagetable walk from 0000000000000144:
(XEN) L4[0x000] = 0000000000000000 ffffffffffffffff
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 6:
(XEN) FATAL PAGE FAULT
(XEN) [error_code=0000]
(XEN) Faulting linear address: 0000000000000144
Reported-by: Cheyenne Wills <cheyenne.wills@gmail.com>
Fixes: e726a82ca0dc ("xen: make gdbsx support configurable")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/common/domctl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 57135d4478..5602dc6b34 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -308,7 +308,9 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
if ( op->domain == DOMID_INVALID )
{
case XEN_DOMCTL_createdomain:
+#ifdef CONFIG_GDBSX
case XEN_DOMCTL_gdbsx_guestmemio:
+#endif
d = NULL;
break;
}
--
2.34.1
On 16.04.2022 15:31, Juergen Gross wrote: > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -308,7 +308,9 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > if ( op->domain == DOMID_INVALID ) > { > case XEN_DOMCTL_createdomain: > +#ifdef CONFIG_GDBSX > case XEN_DOMCTL_gdbsx_guestmemio: > +#endif > d = NULL; > break; > } Wouldn't we be better off simply deleting this case label? dbg_rw_mem() resolves the domid anyway (exactly as done a few lines down from here), so I don't see why we couldn't pass a struct domain * there instead of a domid_t. This would also reduce the risk of further similar "overrides" appearing here (taking existing instances as "excuse"), and breaking things again in a similar way. And finally I think iommu_do_domctl() needs making resilient against d coming in as NULL. This isn't just to cover the issue here, but perhaps more importantly because XEN_DOMCTL_test_assign_device can legitimately end up having NULL passed here (when the caller passed DOMID_INVALID). We've simply been lucky that libxl doesn't use this variant of calling this domctl. I guess when d is NULL we ought to check the global flag there rather than the per-domain one. Jan
On 19.04.22 10:42, Jan Beulich wrote: > On 16.04.2022 15:31, Juergen Gross wrote: >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -308,7 +308,9 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) >> if ( op->domain == DOMID_INVALID ) >> { >> case XEN_DOMCTL_createdomain: >> +#ifdef CONFIG_GDBSX >> case XEN_DOMCTL_gdbsx_guestmemio: >> +#endif >> d = NULL; >> break; >> } > > Wouldn't we be better off simply deleting this case label? dbg_rw_mem() > resolves the domid anyway (exactly as done a few lines down from here), > so I don't see why we couldn't pass a struct domain * there instead of > a domid_t. Seems like a good idea. Will send V2. > This would also reduce the risk of further similar "overrides" appearing > here (taking existing instances as "excuse"), and breaking things again > in a similar way. > > And finally I think iommu_do_domctl() needs making resilient against d > coming in as NULL. This isn't just to cover the issue here, but perhaps > more importantly because XEN_DOMCTL_test_assign_device can legitimately > end up having NULL passed here (when the caller passed DOMID_INVALID). > We've simply been lucky that libxl doesn't use this variant of calling > this domctl. I guess when d is NULL we ought to check the global flag > there rather than the per-domain one. I think this should be another patch, though. Juergen
On 19.04.2022 11:22, Juergen Gross wrote: > On 19.04.22 10:42, Jan Beulich wrote: >> On 16.04.2022 15:31, Juergen Gross wrote: >> And finally I think iommu_do_domctl() needs making resilient against d >> coming in as NULL. This isn't just to cover the issue here, but perhaps >> more importantly because XEN_DOMCTL_test_assign_device can legitimately >> end up having NULL passed here (when the caller passed DOMID_INVALID). >> We've simply been lucky that libxl doesn't use this variant of calling >> this domctl. I guess when d is NULL we ought to check the global flag >> there rather than the per-domain one. > > I think this should be another patch, though. Sure. I'm happy to make one. Jan
© 2016 - 2024 Red Hat, Inc.