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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.06.2026 18:11, Oleksii Kurochko wrote:
> 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.

Imo on arch-es not supporting 2L, domains shouldn't start in 2L mode.

>>>   - 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.

Or, better yet, deal with that as well. But see also below.

>>> --- 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:

We're getting closer. Imo we want to go farther, though: shared_info() as a
construct should be unavailable when !HAS_SHARED_INFO. _That_ then will
make obvious (by causing build failures) that all respective use sites were
properly dealt with.

> --- 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
>   }

I agree with #ifdef here.

> --- 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

While as per above shared_info() would best not exist when !HAS_SHARED_INFO
(in which case #ifdef may be unavoidable here), an alternative where
IS_ENABLED() could be used here may want at least considering. E.g.
causing a link-time failure when shared_info() is used (and not compiled
out).

> --- 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);

Constructs like this are imo somewhat ugly. Using IS_ENABLED() instead
would make things at least a little better (again imo).

Jan