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 6:11 PM, Oleksii Kurochko wrote:
> > >   - 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.
For that part could be considered ...

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

+#ifdef CONFIG_HAS_SHARED_INFO
     info->shared_info_frame =
         gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
     BUG_ON(SHARED_M2P(info->shared_info_frame));
+#else
+    info->shared_info_frame = INVALID_GFN_RAW;
+#endif

~ Oleksii