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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

~Andrew