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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/3/26 1:23 PM, Jan Beulich wrote:
> 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.
If the change will look like:

  if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) && guest_test_bit(d, port, 
&shared_info(d, evtchn_pending)) )
      evtchn->pending = true;

It will help to avoid NULL pointer dereference of shared info page in 
case of 2L isn't supported. But considering that shared_info() macros 
will be reworked in the way which will lead to linkage error in the case 
when it is used and arch doesn't have shared info page support usage of 
shared_info() in setup_ports() will lead to linkage error what is wanted 
to be avoid for arch without 2L support.
> > 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.
I looked at FIFO ops again and it looks like it would be enough just to 
do the following:
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -62,6 +62,9 @@ static inline event_word_t 
*evtchn_fifo_word_from_port(const struct domain *d,
      */
     smp_rmb();

+    if ( unlikely(!d->evtchn_fifo) )
+        return NULL;
+
     if ( unlikely(port >= d->evtchn_fifo->num_evtchns) )
         return NULL;

as evtchn_fifo_word_from_port() is used at the begining of fifo ops and 
if its NULL then it will just return without execution of the rest of an 
operation so even something isn't allocated it is still safe.
The changes above are wrong as evtchn_is_pending(d, evtchn) will be 
called for FIFO's evtchn but the original check is checking if there is 
any pending interrupt in 2L before final switch to FIFO. So we still 
want to have:
+#ifdef CONFIG_HAS_SHARED_INFO
+        if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
+            evtchn->pending = true;
+#endif

And then probably we can d->evtchn_port_op with FIFO ops in the way how 
it is done now instead of:
@@ -1660,7 +1629,7 @@ int evtchn_init(struct domain *d, unsigned int 
max_port)
 #ifdef CONFIG_HAS_SHARED_INFO
     evtchn_2l_init(d);
 #else
+    evtchn_fifo_init_ops(d);
 #endif
     d->max_evtchn_port = min_t(unsigned int, max_port, INT_MAX);

~ Oleksii