Re: [PATCH] xen/vcpu: ensure no write when runstate handler is NULL

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Fri, Oct 23, 2026 at 04:46:42PM +0100, Andrew Cooper wrote:
> On 23/10/2026 2:33 pm, Roger Pau Monne wrote:
> > Both the normal and the compat versions of
> > VCPUOP_register_runstate_memory_area allow registering a NULL handler as a
> > way to signal deregistration of the previous area, and
> > update_runstate_area() will skip the update when the handler is NULL.
> >
> > However when handling the VCPUOP_register_runstate_memory_area hypercall
> > itself there's an unconditional attempt to update the newly registered
> > runstate area, without taking into account whether it points to NULL.  Skip
> > such update if the user-provided handle is NULL.
> >
> > Signed-off-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> > ---
> >  xen/arch/x86/x86_64/domain.c | 4 ++++
> >  xen/common/domain.c          | 4 ++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/xen/arch/x86/x86_64/domain.c b/xen/arch/x86/x86_64/domain.c
> > index a02d4f569ee5..e4d49cbbfeb6 100644
> > --- a/xen/arch/x86/x86_64/domain.c
> > +++ b/xen/arch/x86/x86_64/domain.c
> > @@ -58,6 +58,10 @@ compat_vcpu_op(int cmd, unsigned int vcpuid, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >          rc = 0;
> >          guest_from_compat_handle(v->runstate_guest.compat, area.addr.h);
> >  
> > +        /* NULL handle is deregistering the address. */
> > +        if ( compat_handle_is_null(area.addr.h) )
> > +            break;
> > +
> >          if ( v == current )
> >          {
> >              XLAT_vcpu_runstate_info(&info, &v->runstate);
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 6b52713518da..30a17b8b7edb 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -2330,6 +2330,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >          rc = 0;
> >          runstate_guest(v) = area.addr.h;
> >  
> > +        /* NULL handle is deregistering the address. */
> > +        if ( guest_handle_is_null(area.addr.h) )
> > +            break;
> > +
> >          if ( v == current )
> >          {
> >              __copy_to_guest(runstate_guest(v), &v->runstate, 1);
> 
> This is all horrible.
> 
> I think you ought to add /* Update the newly-registered runstate area,
> best-effort */ to both of these hunks.
> 
> I also think it would be easier to follow with the commit message
> structured in a different order.  I suggest:
> 
> --8<--
> The VCPUOP_register_runstate_memory_area hypercall tries to update the
> newly registered area, even when NULL is being registered.  If the guest
> happens to have memory mapped at 0 in the address space, this write will
> succeed.
> 
> Ever since it's introduction in Xen 3.0, NULL is the initial state of
> the pointer, and guards the update of the runstate area in
> context_switch().  NULL has also been accepted as an input to the hypercall.
> 
> It is unclear whether de-registration of the area was considered at the
> time, but the behaviour of the hypercall has always supported. 
> De-registering using NULL is a common pattern for other hypercalls, so
> formally make this the case for VCPUOP_register_runstate_memory_area by
> skipping the update if the area has been de-registered.
> --8<--
> 
> I think you also want an adjustment to the public header file to say
> that NULL can be used to de-register.
> 
> Thoughts?

LGTM, I will prepare v2 then, thanks a lot for the feedback.

Roger.