Re: [PATCH v2 2/3] xen/domain: fix UBSAN null pointer dereference of d->shared_info

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/2/26 1:19 PM, Jan Beulich wrote:
> On 25.05.2026 15:20, Oleksii Kurochko wrote:
> > It is legal to have d->shared_info equal to NULL for architectures which
> > support only the FIFO ABI for event channel management.
> >
> > Having d->shared_info == NULL leads to a UBSAN issue on such architectures:
> >    UBSAN: Undefined behaviour in common/domain.c:325:10
> >           member access within null pointer of type 'struct shared_info_t'
> >
> > 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.
> > 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.
> >
> > Additionally, if an architecture supports only the FIFO ABI, setup_ports()
> > should be updated to avoid a NULL pointer dereference of d->shared_info,
> > since in that case there will be no pending events in
> > shared_info->evtchn_pending and the pending flag of the FIFO event channel
> > does not need to be set to true.
> > update_domain_wallclock_time() accesses d->shared_info via shared_info()
> > macro. On architectures that do not allocate a shared_info page (currently
> > RISC-V, which runs guests in dom0less mode without the PV ABI), this causes
> > a NULL dereference. The early return is safe: if there is no shared_info
> > page, there is nothing to update. For all existing architectures (x86, ARM)
> > that do allocate it, the guard is never taken and behavior is unchanged.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Update commit message + subject.
> >   - Drop Fixes tag.
> >   - Handle migration of pending events from 2L and FIFO ABIs when arch
> >     support only FIFO ABI.
> What does this item describe? On an arch supporting only FIFO, how could
> evtchn need migrating from 2L?
Agree this item is inaccurate.

evtchn_init() always calls evtchn_2l_init(d) first 
(event_channel.c:1627), so every domain starts with 2L regardless of 
arch (of course, it is just initialization of evtchn_port_ops which 
aren't really used when only FIFO is supported).
setup_ports() is called during the guest-initiated 2L→FIFO transition 
(event_fifo.c:637), not at arch init time. There is no arch that 
supports "only FIFO" as a starting state and that is why it is needed to 
guard setup_ports() against NULL d->shared_info when migrating 2L 
pending state to FIFO even 2L wasn't really used by an arch with only 
FIFO support.
I Will drop this item to not confuse.

> >   - Update the commit message.
> >   - Protect some other places in common code from NULL pointer deref of
> >     d->shared_info.
> What I'm still missing is the description clarifying why other uses don't
> need guarding (or that there simply are no other uses, which - however -
> I doubt).
I will add an explicit paragraph mentioning that the 2L ops in 
event_2l.c are unreachable for a domain with no shared_info.
The only place which isn't covered now is  domctl.c:108 
(virt_to_mfn(d->shared_info)) is only reached via the 
XEN_DOMCTL_getdomaininfo path and
as RISC-V doesn't use it now it could be left as it is what also could 
be added to commit message.
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -320,7 +320,7 @@ void vcpu_info_reset(struct vcpu *v)
> >       struct domain *d = v->domain;
> >   
> >       v->vcpu_info_area.map =
> > -        ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
> > +        ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS && d->shared_info)
> >            ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> >            : &dummy_vcpu_info);
> >   }
> At the example of this: The extra conditionals are dead code on x86 and
> Arm. While the status of the respective Misra rule is still uncertain
> for Xen, imo we'd be better off avoiding the introduction of new dead
> code. Which in turn means we may need some kind of abstraction to have
> these extra conditionals in place only for arch-es not supporting
> shared-info at all.
What about then add config HAS_SHARED_INFO to xen/common/Kconfig and then:

diff --git a/xen/common/domain.c b/xen/common/domain.c
index e64b7df9b704..58442ce1f952 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -319,10 +319,14 @@ void vcpu_info_reset(struct vcpu *v)
 {
     struct domain *d = v->domain;

+#ifdef CONFIG_HAS_SHARED_INFO
     v->vcpu_info_area.map =
-        ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS && d->shared_info)
-         ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
-         : &dummy_vcpu_info);
+        (v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
+        ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
+        : &dummy_vcpu_info;
+#else
+    v->vcpu_info_area.map = &dummy_vcpu_info;
+#endif
 }

 static struct domain *alloc_domain_struct(void)
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 59d9bf4c7ec0..3d7104100f0b 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -562,9 +562,10 @@ static void setup_ports(struct domain *d, unsigned 
int prev_evtchns)
         evtchn = evtchn_from_port(d, port);

-        if ( d->shared_info &&
-             guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
+#ifdef CONFIG_HAS_SHARED_INFO
+        if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
             evtchn->pending = true;
+#endif

         evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
     }
diff --git a/xen/common/time.c b/xen/common/time.c
index 1ee49a8b0d13..da8403949102 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -94,8 +94,9 @@ void update_domain_wallclock_time(struct domain *d)
     uint32_t *wc_version;
     uint64_t sec;

-    if ( !d->shared_info )
-        return;
+#ifndef CONFIG_HAS_SHARED_INFO
+    return;
+#endif

     spin_lock(&wc_lock);

Thanks.

~ Oleksii