[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



It is legal to have d->shared_info equal to NULL for architectures which
support only the FIFO ABI for event channel management.

Having d->shared_info == NULL leads to a UBSAN issue on such architectures:
  UBSAN: Undefined behaviour in common/domain.c:325:10
         member access within null pointer of type 'struct shared_info_t'

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

Additionally, if an architecture supports only the FIFO ABI, setup_ports()
should be updated to avoid a NULL pointer dereference of d->shared_info,
since in that case there will be no pending events in
shared_info->evtchn_pending and the pending flag of the FIFO event channel
does not need to be set to true.
update_domain_wallclock_time() accesses d->shared_info via shared_info()
macro. On architectures that do not allocate a shared_info page (currently
RISC-V, which runs guests in dom0less mode without the PV ABI), this causes
a NULL dereference. The early return is safe: if there is no shared_info
page, there is nothing to update. For all existing architectures (x86, ARM)
that do allocate it, the guard is never taken and behavior is unchanged.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
---
Changes in v2:
 - Update commit message + subject.
 - Drop Fixes tag.
 - Handle migration of pending events from 2L and FIFO ABIs when arch
   support only FIFO ABI.
 - Update the commit message.
 - Protect some other places in common code from NULL pointer deref of
   d->shared_info.
 - Drop R-by: Baptiste ... as some extra checks are added.
---
 xen/common/domain.c     | 2 +-
 xen/common/event_fifo.c | 3 ++-
 xen/common/time.c       | 3 +++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index bb9e210c2895..e64b7df9b704 100644
--- 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);
 }
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 37cba9bc4564..59d9bf4c7ec0 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -562,7 +562,8 @@ static void setup_ports(struct domain *d, unsigned int 
prev_evtchns)
 
         evtchn = evtchn_from_port(d, port);
 
-        if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
+        if ( d->shared_info &&
+             guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
             evtchn->pending = true;
 
         evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
diff --git a/xen/common/time.c b/xen/common/time.c
index 04a65f00b35c..1ee49a8b0d13 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -94,6 +94,9 @@ void update_domain_wallclock_time(struct domain *d)
     uint32_t *wc_version;
     uint64_t sec;
 
+    if ( !d->shared_info )
+        return;
+
     spin_lock(&wc_lock);
 
     wc_version = &shared_info(d, wc_version);
-- 
2.54.0




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.