[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 2/3] xen/domain: fix UBSAN null pointer dereference in vcpu_info_reset()
On 5/19/26 1:56 PM, 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).
I'm not familiar with the FIFO ABI, but after a quick look it seems
d->shared_info is used here for example:
static void setup_ports(struct domain *d, unsigned int prev_evtchns)
{
...
for ( port = 1; port < prev_evtchns; port++ )
{
...
evtchn = evtchn_from_port(d, port);
if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
evtchn->pending = true;
evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
}
}
So shouldn't it be still allocated in arch_domain_create() or as I
suggested in domain_create()?
~ Oleksii
|
