| Message ID | 20220416133158.16162-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fix XEN_DOMCTL_gdbsx_guestmemio crash | expand |
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
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; }
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(+)