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

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

[Roger Pau Monne]

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.

Fixes: 705ea2b54c35 ("New VCPUOP_register_runstate_memory_area hypercall.")
Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
Changes since v1:
 - Reword commit message.
 - Comment on the runstate update attempt done in common_vcpu_op().
 - Notice in the public headers that passing NULL deregisters any previous
   region.
---
 xen/arch/x86/x86_64/domain.c | 5 +++++
 xen/common/domain.c          | 5 +++++
 xen/include/public/vcpu.h    | 1 +
 3 files changed, 11 insertions(+)

diff --git a/xen/arch/x86/x86_64/domain.c b/xen/arch/x86/x86_64/domain.c
index a02d4f569ee5..533b0b21a00e 100644
--- a/xen/arch/x86/x86_64/domain.c
+++ b/xen/arch/x86/x86_64/domain.c
@@ -58,6 +58,11 @@ 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;
+
+        /* Update the newly-registered runstate area, best-effort. */
         if ( v == current )
         {
             XLAT_vcpu_runstate_info(&info, &v->runstate);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6b52713518da..a1e4cb80871c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -2330,6 +2330,11 @@ 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;
+
+        /* Update the newly-registered runstate area, best-effort. */
         if ( v == current )
         {
             __copy_to_guest(runstate_guest(v), &v->runstate, 1);
diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
index f7445ac0b08a..403b81457c82 100644
--- a/xen/include/public/vcpu.h
+++ b/xen/include/public/vcpu.h
@@ -112,6 +112,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_t);
  *     VCPU was last scheduled to run.
  *  3. New code wants to prefer VCPUOP_register_runstate_phys_area, and only
  *     fall back to the operation here for backwards compatibility.
+ *  4. Passing a NULL handle will deregister the region.
  * @extra_arg == pointer to vcpu_register_runstate_memory_area structure.
  */
 #define VCPUOP_register_runstate_memory_area 5
-- 
2.55.0

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

[Andrew Cooper]

On 23/10/2026 5:17 pm, Roger Pau Monne wrote:
> 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.

"supported this."

Sorry, I had a typo.

> 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.
>
> Fixes: 705ea2b54c35 ("New VCPUOP_register_runstate_memory_area hypercall.")
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Jan Beulich]

On 23.10.2026 18:17, Roger Pau Monne wrote:
> --- a/xen/arch/x86/x86_64/domain.c
> +++ b/xen/arch/x86/x86_64/domain.c
> @@ -58,6 +58,11 @@ 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;
> +
> +        /* Update the newly-registered runstate area, best-effort. */
>          if ( v == current )
>          {
>              XLAT_vcpu_runstate_info(&info, &v->runstate);

While likely unnecessary in this x86-only handling, ...

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -2330,6 +2330,11 @@ 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;
> +
> +        /* Update the newly-registered runstate area, best-effort. */
>          if ( v == current )
>          {
>              __copy_to_guest(runstate_guest(v), &v->runstate, 1);

... if we make null an official "de-register", then imo the earlier
guest_handle_okay() also wants bypassing in the null case. An arch
could - in principle - reject a null handle.

Jan

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

[Roger Pau Monné]

On Mon, Oct 26, 2026 at 02:06:48PM +0100, Jan Beulich wrote:
> On 23.10.2026 18:17, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/x86_64/domain.c
> > +++ b/xen/arch/x86/x86_64/domain.c
> > @@ -58,6 +58,11 @@ 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;
> > +
> > +        /* Update the newly-registered runstate area, best-effort. */
> >          if ( v == current )
> >          {
> >              XLAT_vcpu_runstate_info(&info, &v->runstate);
> 
> While likely unnecessary in this x86-only handling, ...
> 
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -2330,6 +2330,11 @@ 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;
> > +
> > +        /* Update the newly-registered runstate area, best-effort. */
> >          if ( v == current )
> >          {
> >              __copy_to_guest(runstate_guest(v), &v->runstate, 1);
> 
> ... if we make null an official "de-register", then imo the earlier
> guest_handle_okay() also wants bypassing in the null case. An arch
> could - in principle - reject a null handle.

Yes, it could en theory be possible, albeit I think it's very unlikely
we gain anything similar to x86 PV elsewhere.  Would you be OK with:

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6b52713518da..b41be89cd0b9 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -2324,12 +2324,21 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&area, arg, 1) )
             break;

+        /* NULL handle is deregistering the address. */
+        if ( guest_handle_is_null(area.addr.h) )
+        {
+            runstate_guest(v) = area.addr.h;
+            rc = 0;
+            break;
+        }
+
         if ( !guest_handle_okay(area.addr.h, 1) )
             break;

         rc = 0;
         runstate_guest(v) = area.addr.h;

+        /* Update the newly-registered runstate area, best-effort. */
         if ( v == current )
         {
             __copy_to_guest(runstate_guest(v), &v->runstate, 1);

Thanks, Roger.

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

[Jan Beulich]

On 26.10.2026 14:58, Roger Pau Monné wrote:
> On Mon, Oct 26, 2026 at 02:06:48PM +0100, Jan Beulich wrote:
>> On 23.10.2026 18:17, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/x86_64/domain.c
>>> +++ b/xen/arch/x86/x86_64/domain.c
>>> @@ -58,6 +58,11 @@ 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;
>>> +
>>> +        /* Update the newly-registered runstate area, best-effort. */
>>>          if ( v == current )
>>>          {
>>>              XLAT_vcpu_runstate_info(&info, &v->runstate);
>>
>> While likely unnecessary in this x86-only handling, ...
>>
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -2330,6 +2330,11 @@ 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;
>>> +
>>> +        /* Update the newly-registered runstate area, best-effort. */
>>>          if ( v == current )
>>>          {
>>>              __copy_to_guest(runstate_guest(v), &v->runstate, 1);
>>
>> ... if we make null an official "de-register", then imo the earlier
>> guest_handle_okay() also wants bypassing in the null case. An arch
>> could - in principle - reject a null handle.
> 
> Yes, it could en theory be possible, albeit I think it's very unlikely
> we gain anything similar to x86 PV elsewhere.  Would you be OK with:

Yes, this is fine with me:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -2324,12 +2324,21 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&area, arg, 1) )
>              break;
> 
> +        /* NULL handle is deregistering the address. */
> +        if ( guest_handle_is_null(area.addr.h) )
> +        {
> +            runstate_guest(v) = area.addr.h;
> +            rc = 0;
> +            break;
> +        }
> +
>          if ( !guest_handle_okay(area.addr.h, 1) )
>              break;
> 
>          rc = 0;
>          runstate_guest(v) = area.addr.h;
> 
> +        /* Update the newly-registered runstate area, best-effort. */
>          if ( v == current )
>          {
>              __copy_to_guest(runstate_guest(v), &v->runstate, 1);
> 
> Thanks, Roger.