Re: [PATCH v1 2/3] xen/domain: fix UBSAN null pointer dereference in vcpu_info_reset()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/05/2026 1:06 pm, Jan Beulich wrote:
> On 19.05.2026 13:56, Andrew Cooper wrote:
>> On 19/05/2026 12:51 pm, Jan Beulich wrote:
>>> On 19.05.2026 13:32, Andrew Cooper wrote:
>>>> On 19/05/2026 12:22 pm, Oleksii Kurochko wrote:
>>>>> On 5/19/26 12:55 PM, Oleksii Kurochko wrote:
>>>>>> On 5/19/26 11:37 AM, Jan Beulich wrote:
>>>>>>> On 19.05.2026 10:39, Oleksii Kurochko wrote:
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> However, it does not guard against d->shared_info being NULL.  The
>>>>>>>> shared_info() macro expands to a member access through d->shared_info,
>>>>>>>> so when an architecture does not allocate a shared_info page the
>>>>>>>> dereference triggers UBSAN:
>>>>>>>>    UBSAN: Undefined behaviour in common/domain.c:325:10
>>>>>>>>    member access within null pointer of type 'struct shared_info_t'
>>>>>>>>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Fixes: 295514ff75506 ("common: convert vCPU info area registration")
>>>>>>> I question this, largely (but not only) because I also ...
>>>>>>>
>>>>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>>>>>> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> RISC-V does not allocate a shared_info page at the momemnt because its
>>>>>>>> guests run in dom0less mode and do not use the Xen PV ABI, so
>>>>>>>> d->shared_info remains NULL throughout domain lifetime.
>>>>>>> ... question this mode of operation. Yes, you may (for now) be able
>>>>>>> to get
>>>>>>> away without, but e.g. event channels will want supporting at some
>>>>>>> point.
>>>>>>> Which will require a shared info page. Better put that in place
>>>>>>> right away,
>>>>>>> even if the guests you test with don't use it (yet). Certain other
>>>>>>> common
>>>>>>> code also assumes d->shared_info to never be NULL for an alive domain.
>>>>>>>
>>>>>> Would it be fine than to allocate it in arch_domain_create() ... :
>>>>>>
>>>>>>      if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
>>>>>>          goto fail;
>>>>>>
>>>>>>      clear_page(d->shared_info);
>>>>>>
>>>>>> ... but without calling share_xen_page_with_guest() after that
>>>>>> allocation as share_xen_page_with_guest() isn't implemented at the
>>>>>> moment?
>>>>> Or could it be an option for all arch-s move allocation of
>>>>> d->shared_info to domain_create() in common just after
>>>>> arch_domain_create()?
>>>>>
>>>>> The only question if share_xen_page_with_guest() could be ifdef-ed
>>>>> somehow so not to block new ports to implement it from the start.
>>>> shared_info is an x86-PV-ism which escaped into HVM and then infected
>>>> ARM too.
>>>>
>>>> Sadly it's ABI there, but this is one of many areas where I really want
>>>> RISC-V not to inherit the mistakes of prior ports.
>>> In which case, how do you propose e.g. event channels to be handled in
>>> whatever is going to be the alternative?
>> Implement proper enumeration of virtual capabilities (to be retrofitted
>> to x86/ARM too), and only offer the FIFO ABI (which is superior in every
>> way to the 2L ABI).
> What about the wc_* fields then?

Well - ARM seems to have the right idea by entirely ignoring them and
leaving it all 0.

At least it's obviously got no data in it, as opposed to what we do on
x86 where we pretend it's possible to use some stale value to determine
the wallclock time without even a tied TSC reference.

This is one of several things contributing to our in-guest timekeeping bugs.

>  And about everything in struct arch_shared_info?

There is nothing else in any arch_shared_info applicable outside of PV
guests.

~Andrew