[v3] x86/domain: adjust limitation on shared_info allocation below 4G

[PATCH v3] x86/domain: adjust limitation on shared_info allocation below 4G

Posted by Roger Pau Monne 2 days, 17 hours ago

The limitation of shared_info being allocated below 4G to fit in the
start_info field only applies to 32bit PV guests.  On 64bit PV guests the
start_info field is 64bits wide.  HVM guests don't use start_info at all.

Drop the restriction in arch_domain_create() and instead free and
re-allocate the page from memory below 4G if needed in switch_compat(),
when the guest is set to run in 32bit PV mode.

Fixes: 3cadc0469d5c ("x86_64: shared_info must be allocated below 4GB as it is advertised to 32-bit guests via a 32-bit machine address field in start_info.")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v2:
 - Move/reword some comments.
 - Add spaces in for_each_vcpu call.
---
 xen/arch/x86/domain.c    |  6 +-----
 xen/arch/x86/pv/domain.c | 28 ++++++++++++++++++++++++++++
 xen/common/domain.c      |  2 +-
 xen/include/xen/domain.h |  2 ++
 4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index edb76366b596..8b2f33f1a06c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -881,11 +881,7 @@ int arch_domain_create(struct domain *d,
     if ( d->arch.ioport_caps == NULL )
         goto fail;
 
-    /*
-     * The shared_info machine address must fit in a 32-bit field within a
-     * 32-bit guest's start_info structure. Hence we specify MEMF_bits(32).
-     */
-    if ( (d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32))) == NULL )
+    if ( (d->shared_info = alloc_xenheap_page()) == NULL )
         goto fail;
 
     clear_page(d->shared_info);
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 01499582d2d6..e3273b49269d 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -247,6 +247,34 @@ int switch_compat(struct domain *d)
     d->arch.has_32bit_shinfo = 1;
     d->arch.pv.is_32bit = true;
 
+    /*
+     * For 32bit PV guests the shared_info machine address must fit in a 32-bit
+     * field within the guest's start_info structure.  We might need to free
+     * the current page and allocate a new one that fulfills this requirement.
+     */
+    if ( virt_to_maddr(d->shared_info) >> 32 )
+    {
+        shared_info_t *prev = d->shared_info;
+
+        d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
+        if ( !d->shared_info )
+        {
+            d->shared_info = prev;
+            rc = -ENOMEM;
+            goto undo_and_fail;
+        }
+        put_page(virt_to_page(prev));
+        clear_page(d->shared_info);
+        share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
+        /*
+         * Ensure all pointers to the old shared_info page are replaced.  vCPUs
+         * below XEN_LEGACY_MAX_VCPUS will stash a pointer to
+         * shared_info->vcpu_info[id].
+         */
+        for_each_vcpu ( d, v )
+            vcpu_info_reset(v);
+    }
+
     for_each_vcpu( d, v )
     {
         if ( (rc = setup_compat_arg_xlat(v)) ||
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 163f7fc96658..4f6977c67aa7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -305,7 +305,7 @@ static void vcpu_check_shutdown(struct vcpu *v)
     spin_unlock(&d->shutdown_lock);
 }
 
-static void vcpu_info_reset(struct vcpu *v)
+void vcpu_info_reset(struct vcpu *v)
 {
     struct domain *d = v->domain;
 
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 273717c31b3f..52cdf012c491 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -83,6 +83,8 @@ void cf_check free_pirq_struct(void *ptr);
 int  arch_vcpu_create(struct vcpu *v);
 void arch_vcpu_destroy(struct vcpu *v);
 
+void vcpu_info_reset(struct vcpu *v);
+
 int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
                    struct guest_area *area,
                    void (*populate)(void *dst, struct vcpu *v));
-- 
2.51.0

Re: [PATCH v3] x86/domain: adjust limitation on shared_info allocation below 4G

Posted by Andrew Cooper 2 days, 6 hours ago

On 05/02/2026 8:03 am, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index 01499582d2d6..e3273b49269d 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -247,6 +247,34 @@ int switch_compat(struct domain *d)
>      d->arch.has_32bit_shinfo = 1;
>      d->arch.pv.is_32bit = true;
>  
> +    /*
> +     * For 32bit PV guests the shared_info machine address must fit in a 32-bit
> +     * field within the guest's start_info structure.  We might need to free
> +     * the current page and allocate a new one that fulfills this requirement.
> +     */
> +    if ( virt_to_maddr(d->shared_info) >> 32 )
> +    {
> +        shared_info_t *prev = d->shared_info;
> +
> +        d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
> +        if ( !d->shared_info )
> +        {
> +            d->shared_info = prev;
> +            rc = -ENOMEM;
> +            goto undo_and_fail;
> +        }
> +        put_page(virt_to_page(prev));
> +        clear_page(d->shared_info);
> +        share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
> +        /*
> +         * Ensure all pointers to the old shared_info page are replaced.  vCPUs
> +         * below XEN_LEGACY_MAX_VCPUS will stash a pointer to
> +         * shared_info->vcpu_info[id].
> +         */
> +        for_each_vcpu ( d, v )
> +            vcpu_info_reset(v);

Sorry, I missed something.  Reading this in full, there's an obvious
(risk of) UAF.

put_page(virt_to_page(prev)) needs to be no earlier than here, or we've
freed a page that we still have pointers pointing at.

In practice, I expect that the global domctl lock protects us from
anything actually going wrong.

Nevertheless, for the sake of reordering the actions in this block, lets
just fix it.

~Andrew

> +    }
> +
>      for_each_vcpu( d, v )
>      {
>          if ( (rc = setup_compat_arg_xlat(v)) ||
>

Re: [PATCH v3] x86/domain: adjust limitation on shared_info allocation below 4G

Posted by Roger Pau Monné 2 days, 6 hours ago

On Thu, Feb 05, 2026 at 06:31:04PM +0000, Andrew Cooper wrote:
> On 05/02/2026 8:03 am, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> > index 01499582d2d6..e3273b49269d 100644
> > --- a/xen/arch/x86/pv/domain.c
> > +++ b/xen/arch/x86/pv/domain.c
> > @@ -247,6 +247,34 @@ int switch_compat(struct domain *d)
> >      d->arch.has_32bit_shinfo = 1;
> >      d->arch.pv.is_32bit = true;
> >  
> > +    /*
> > +     * For 32bit PV guests the shared_info machine address must fit in a 32-bit
> > +     * field within the guest's start_info structure.  We might need to free
> > +     * the current page and allocate a new one that fulfills this requirement.
> > +     */
> > +    if ( virt_to_maddr(d->shared_info) >> 32 )
> > +    {
> > +        shared_info_t *prev = d->shared_info;
> > +
> > +        d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
> > +        if ( !d->shared_info )
> > +        {
> > +            d->shared_info = prev;
> > +            rc = -ENOMEM;
> > +            goto undo_and_fail;
> > +        }
> > +        put_page(virt_to_page(prev));
> > +        clear_page(d->shared_info);
> > +        share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
> > +        /*
> > +         * Ensure all pointers to the old shared_info page are replaced.  vCPUs
> > +         * below XEN_LEGACY_MAX_VCPUS will stash a pointer to
> > +         * shared_info->vcpu_info[id].
> > +         */
> > +        for_each_vcpu ( d, v )
> > +            vcpu_info_reset(v);
> 
> Sorry, I missed something.  Reading this in full, there's an obvious
> (risk of) UAF.
> 
> put_page(virt_to_page(prev)) needs to be no earlier than here, or we've
> freed a page that we still have pointers pointing at.
> 
> In practice, I expect that the global domctl lock protects us from
> anything actually going wrong.
> 
> Nevertheless, for the sake of reordering the actions in this block, lets
> just fix it.

Yeah, I've already made that change.  At this point in the domain life
cycle and with d->tot_pages == 0 and the domctl lock held, the vcpu
info area shouldn't be updated in parallel to the bitnewss being
changed, but better make this in the right order.

Thanks for spotting, Roger.

Re: [PATCH v3] x86/domain: adjust limitation on shared_info allocation below 4G

Posted by Andrew Cooper 2 days, 13 hours ago

On 05/02/2026 8:03 am, Roger Pau Monne wrote:
> The limitation of shared_info being allocated below 4G to fit in the
> start_info field only applies to 32bit PV guests.  On 64bit PV guests the
> start_info field is 64bits wide.  HVM guests don't use start_info at all.
>
> Drop the restriction in arch_domain_create() and instead free and
> re-allocate the page from memory below 4G if needed in switch_compat(),
> when the guest is set to run in 32bit PV mode.
>
> Fixes: 3cadc0469d5c ("x86_64: shared_info must be allocated below 4GB as it is advertised to 32-bit guests via a 32-bit machine address field in start_info.")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH v3] x86/domain: adjust limitation on shared_info allocation below 4G

Posted by Jan Beulich 2 days, 16 hours ago

On 05.02.2026 09:03, Roger Pau Monne wrote:
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -247,6 +247,34 @@ int switch_compat(struct domain *d)
>      d->arch.has_32bit_shinfo = 1;
>      d->arch.pv.is_32bit = true;
>  
> +    /*
> +     * For 32bit PV guests the shared_info machine address must fit in a 32-bit
> +     * field within the guest's start_info structure.  We might need to free
> +     * the current page and allocate a new one that fulfills this requirement.
> +     */
> +    if ( virt_to_maddr(d->shared_info) >> 32 )
> +    {
> +        shared_info_t *prev = d->shared_info;
> +
> +        d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
> +        if ( !d->shared_info )
> +        {
> +            d->shared_info = prev;
> +            rc = -ENOMEM;
> +            goto undo_and_fail;
> +        }
> +        put_page(virt_to_page(prev));
> +        clear_page(d->shared_info);
> +        share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
> +        /*
> +         * Ensure all pointers to the old shared_info page are replaced.  vCPUs
> +         * below XEN_LEGACY_MAX_VCPUS will stash a pointer to
> +         * shared_info->vcpu_info[id].
> +         */

To cope with the toolstack-dependent ordering of actions, may I suggest "... may
have stashed ..." instead?

Jan

> +        for_each_vcpu ( d, v )
> +            vcpu_info_reset(v);
> +    }
> +
>      for_each_vcpu( d, v )
>      {
>          if ( (rc = setup_compat_arg_xlat(v)) ||