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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.06.2026 13:05, Oleksii Kurochko wrote:
> 
> 
> On 6/3/26 7:54 AM, Jan Beulich wrote:
>>> --- 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).
> 
> We still want here to have #ifdef instead of IS_ENABLED() as 
> shared_info() shouldn't exist for arch without 2L support so it will end 
> with linkage error.

I don't understand this part.

> Considering that setup_ports() will be called for 
> such arch we have to avoid this part from compilation.
> 
> Alternative is that considering that I suggested in prev emails to 
> introduced stubs for arch which doesn't use 2L:
> 
> +#ifndef CONFIG_HAS_SHARED_INFO
> +static void cf_check evtchn_none_set_pending(
> +    struct vcpu *v, struct evtchn *evtchn) {}
> +static void cf_check evtchn_none_clear_pending(
> +    struct domain *d, struct evtchn *evtchn) {}
> +static void cf_check evtchn_none_unmask(
> +    struct domain *d, struct evtchn *evtchn) {}
> +static bool cf_check evtchn_none_is_pending(
> +    const struct domain *d, const struct evtchn *evtchn) { return false; }
> +static bool cf_check evtchn_none_is_masked(
> +    const struct domain *d, const struct evtchn *evtchn) { return true; }
> +static void cf_check evtchn_none_print_state(
> +    struct domain *d, const struct evtchn *evtchn) {}

This set can be shrunk. For example, the same stub can be used for
clear-pending and unmask. For is-pending and is-masked, considering
that the precise return value shouldn't matter, a single function
(returning false) would likely be good enough as well.

> +static const struct evtchn_port_ops evtchn_port_ops_none = {
> +    .set_pending   = evtchn_none_set_pending,
> +    .clear_pending = evtchn_none_clear_pending,
> +    .unmask        = evtchn_none_unmask,
> +    .is_pending    = evtchn_none_is_pending,
> +    .is_masked     = evtchn_none_is_masked,
> +    .print_state   = evtchn_none_print_state,
> +};
> +
> +static void evtchn_none_init(struct domain *d)
> +{
> +    d->evtchn_port_ops = &evtchn_port_ops_none;
> +}
> +#endif
> 
> For arch without 2L supports .is_pending() will return false we can just 
> do the following instead of ifdef:
> 
> -#ifdef CONFIG_HAS_SHARED_INFO
> -        if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
> -            evtchn->pending = true;
> -#endif
> +        if ( evtchn_is_pending(d, evtchn) )
> +             evtchn->pending = true;
> 
> Would you be okay with this approach instead of ifdef?

I can't really say ahead of seeing the full result (and without it being
made clear why FIFO ops can't be put in place right away, with perhaps a
few small adjustments to the handlers). While this isn't going to be
used for x86, introduction of new cf_check functions always worries me,
at least some.

Jan