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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.