| Message ID | 20250317230806.1179478-1-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: vm_event: do not do vm_event_op for an invalid domain | expand |
On 17/03/2025 11:08 pm, Volodymyr Babchuk wrote: > A privileged domain can issue XEN_DOMCTL_vm_event_op with > op->domain == DOMID_INVALID. In this case vm_event_domctl() > function will get NULL as the first parameter and this will > cause hypervisor panic, as it tries to derefer this pointer. > > Fix the issue by checking if valid domain is passed in. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > This issue was found by the xen fuzzer ([1]) > > [1] https://lore.kernel.org/all/20250315003544.1101488-1-volodymyr_babchuk@epam.com/ > --- > xen/common/vm_event.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c > index fbf1aa0848..a4c233de52 100644 > --- a/xen/common/vm_event.c > +++ b/xen/common/vm_event.c > @@ -600,6 +600,13 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec) > return 0; > } > > + if ( unlikely(!d) ) > + { > + gdprintk(XENLOG_INFO, > + "Tried to do a memory event op on invalid domain\n"); > + return -EINVAL; > + } > + > rc = xsm_vm_event_control(XSM_PRIV, d, vec->mode, vec->op); > if ( rc ) > return rc; Oops. Git blame says this is my fault. This behaviour is intentional. See XEN_DOMCTL_vm_event_op (along with test_assign_device and get_domain_state) early in do_domctl(). It was introduced in commit d48e1836074c ("vm_event: Add a new opcode to get VM_EVENT_INTERFACE_VERSION") so that XEN_VM_EVENT_GET_VERSION could succeed. Apparently I deleted it in commit 48b84249459f ("xen/vm-event: Drop unused u_domctl parameter from vm_event_domctl()"), and that wasn't intentional. That will want putting in with an extra comment. /* All other subops need to target a real domain. */ if ( unlikely(d == NULL) ) return -ESRCH; Don't both with a printk(). It's just noise, and -ESRCH is correct code to use. ~Andrew
On Mon, Mar 17, 2025 at 7:08 PM Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote: > > A privileged domain can issue XEN_DOMCTL_vm_event_op with > op->domain == DOMID_INVALID. In this case vm_event_domctl() > function will get NULL as the first parameter and this will > cause hypervisor panic, as it tries to derefer this pointer. > > Fix the issue by checking if valid domain is passed in. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > This issue was found by the xen fuzzer ([1]) > > [1] https://lore.kernel.org/all/20250315003544.1101488-1-volodymyr_babchuk@epam.com/ > --- > xen/common/vm_event.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c > index fbf1aa0848..a4c233de52 100644 > --- a/xen/common/vm_event.c > +++ b/xen/common/vm_event.c > @@ -600,6 +600,13 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec) > return 0; > } > > + if ( unlikely(!d) ) > + { > + gdprintk(XENLOG_INFO, > + "Tried to do a memory event op on invalid domain\n"); This is not a memory event op? Tamas
Hi Tamas, Tamas K Lengyel <tamas@tklengyel.com> writes: > On Mon, Mar 17, 2025 at 7:08 PM Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> wrote: >> >> A privileged domain can issue XEN_DOMCTL_vm_event_op with >> op->domain == DOMID_INVALID. In this case vm_event_domctl() >> function will get NULL as the first parameter and this will >> cause hypervisor panic, as it tries to derefer this pointer. >> >> Fix the issue by checking if valid domain is passed in. >> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> >> --- >> >> This issue was found by the xen fuzzer ([1]) >> >> [1] https://lore.kernel.org/all/20250315003544.1101488-1-volodymyr_babchuk@epam.com/ >> --- >> xen/common/vm_event.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c >> index fbf1aa0848..a4c233de52 100644 >> --- a/xen/common/vm_event.c >> +++ b/xen/common/vm_event.c >> @@ -600,6 +600,13 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec) >> return 0; >> } >> >> + if ( unlikely(!d) ) >> + { >> + gdprintk(XENLOG_INFO, >> + "Tried to do a memory event op on invalid domain\n"); > > This is not a memory event op? Oh, this is good catch. I absent mindedly copied an error message from a couple of lines below. Looks like we need another patch that fixes error messages. -- WBR, Volodymyr
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index fbf1aa0848..a4c233de52 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -600,6 +600,13 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec) return 0; } + if ( unlikely(!d) ) + { + gdprintk(XENLOG_INFO, + "Tried to do a memory event op on invalid domain\n"); + return -EINVAL; + } + rc = xsm_vm_event_control(XSM_PRIV, d, vec->mode, vec->op); if ( rc ) return rc;
A privileged domain can issue XEN_DOMCTL_vm_event_op with op->domain == DOMID_INVALID. In this case vm_event_domctl() function will get NULL as the first parameter and this will cause hypervisor panic, as it tries to derefer this pointer. Fix the issue by checking if valid domain is passed in. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- This issue was found by the xen fuzzer ([1]) [1] https://lore.kernel.org/all/20250315003544.1101488-1-volodymyr_babchuk@epam.com/ --- xen/common/vm_event.c | 7 +++++++ 1 file changed, 7 insertions(+)