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

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

[Roger Pau Monne]

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@xenproject.org>
---
 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);
-- 
2.55.0

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

[Roger Pau Monné]

On Fri, Oct 23, 2026 at 03:33:16PM +0200, 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.

I've added:

Fixes: 705ea2b54c35 ("New VCPUOP_register_runstate_memory_area hypercall.")

Roger.

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

[Andrew Cooper]

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@xenproject.org>
> ---
>  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?

~Andrew

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

[Roger Pau Monné]

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@xenproject.org>
> > ---
> >  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.