Re: [PATCH v1 2/3] xen/domain: fix UBSAN null pointer dereference in vcpu_info_reset()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/19/26 1:32 PM, Andrew Cooper wrote:
> On 19/05/2026 12:22 pm, Oleksii Kurochko wrote:
> > On 5/19/26 12:55 PM, Oleksii Kurochko wrote:
> > > On 5/19/26 11:37 AM, Jan Beulich wrote:
> > > > On 19.05.2026 10:39, Oleksii Kurochko wrote:
> > > > > vcpu_info_reset() maps v->vcpu_info_area.map to the per-vcpu slot
> > > > > inside
> > > > > the domain's shared_info page for vcpus with id <
> > > > > XEN_LEGACY_MAX_VCPUS,
> > > > > and falls back to dummy_vcpu_info for vcpus beyond that limit.
> > > > >
> > > > > However, it does not guard against d->shared_info being NULL.  The
> > > > > shared_info() macro expands to a member access through d->shared_info,
> > > > > so when an architecture does not allocate a shared_info page the
> > > > > dereference triggers UBSAN:
> > > > >     UBSAN: Undefined behaviour in common/domain.c:325:10
> > > > >     member access within null pointer of type 'struct shared_info_t'
> > > > >
> > > > > Extend the existing fallback condition to also cover the case where no
> > > > > shared_info page has been allocated, mapping the vcpu to
> > > > > dummy_vcpu_info
> > > > > instead. This is the correct behaviour: dummy_vcpu_info already serves
> > > > > as the safe stand-in for vcpus that have no usable shared_info slot.
> > > > >
> > > > > Fixes: 295514ff75506 ("common: convert vCPU info area registration")
> > > > I question this, largely (but not only) because I also ...
> > > >
> > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > > > Reviewed-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> > > > > ---
> > > > > RISC-V does not allocate a shared_info page at the momemnt because its
> > > > > guests run in dom0less mode and do not use the Xen PV ABI, so
> > > > > d->shared_info remains NULL throughout domain lifetime.
> > > > ... question this mode of operation. Yes, you may (for now) be able
> > > > to get
> > > > away without, but e.g. event channels will want supporting at some
> > > > point.
> > > > Which will require a shared info page. Better put that in place
> > > > right away,
> > > > even if the guests you test with don't use it (yet). Certain other
> > > > common
> > > > code also assumes d->shared_info to never be NULL for an alive domain.
> > > Would it be fine than to allocate it in arch_domain_create() ... :
> > >
> > >       if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
> > >           goto fail;
> > >
> > >       clear_page(d->shared_info);
> > >
> > > ... but without calling share_xen_page_with_guest() after that
> > > allocation as share_xen_page_with_guest() isn't implemented at the
> > > moment?
> > Or could it be an option for all arch-s move allocation of
> > d->shared_info to domain_create() in common just after
> > arch_domain_create()?
> >
> > The only question if share_xen_page_with_guest() could be ifdef-ed
> > somehow so not to block new ports to implement it from the start.
> shared_info is an x86-PV-ism which escaped into HVM and then infected
> ARM too.
>
> Sadly it's ABI there, but this is one of many areas where I really want
> RISC-V not to inherit the mistakes of prior ports.
Could you please clarify what could be done better now? It seems like 
shared_info is used in a lot of places in common code base and it is 
needed for event channel stuff for example.
~ Oleksii