Xen Security Advisory 107 - Mishandling of uninitialised FIFO-based event channel control blocks

evtchn: check control block exists when using FIFO-based events

When using the FIFO-based event channels, there are no checks for the
existance of a control block when binding an event or moving it to a
different VCPU.  This is because events may be bound when the ABI is
in 2-level mode (e.g., by the toolstack before the domain is started).

The guest may trigger a Xen crash in evtchn_fifo_set_pending() if:

  a) the event is bound to a VCPU without a control block; or
  b) VCPU 0 does not have a control block.

In case (a), Xen will crash when looking up the current queue.  In
(b), Xen will crash when looking up the old queue (which defaults to a
queue on VCPU 0).

By allocating all the per-VCPU structures when enabling the FIFO ABI,
we can be sure that v->evtchn_fifo is always valid.

EVTCHNOP_init_control_block for all the other CPUs need only map the
shared control block.

A single check in evtchn_fifo_set_pending() before accessing the
control block fixes all cases where the guest has not initialized some
control blocks.

This is XSA-107.

Reported-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: David Vrabel <[email protected]>
Reviewed-by: Jan Beulich <[email protected]>

--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -178,6 +178,19 @@ static void evtchn_fifo_set_pending(stru
         bool_t linked = 0;
 
         /*
+         * Control block not mapped.  The guest must not unmask an
+         * event until the control block is initialized, so we can
+         * just drop the event.
+         */
+        if ( unlikely(!v->evtchn_fifo->control_block) )
+        {
+            printk(XENLOG_G_WARNING
+                   "d%dv%d has no FIFO event channel control block\n",
+                   d->domain_id, v->vcpu_id);
+            goto done;
+        }
+
+        /*
          * No locking around getting the queue. This may race with
          * changing the priority but we are allowed to signal the
          * event once on the old priority.
@@ -385,36 +398,42 @@ static void init_queue(struct vcpu *v, s
 {
     spin_lock_init(&q->lock);
     q->priority = i;
-    q->head = &v->evtchn_fifo->control_block->head[i];
 }
 
-static int setup_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
+static int setup_control_block(struct vcpu *v)
 {
-    struct domain *d = v->domain;
     struct evtchn_fifo_vcpu *efv;
-    void *virt;
     unsigned int i;
-    int rc;
-
-    if ( v->evtchn_fifo )
-        return -EINVAL;
 
     efv = xzalloc(struct evtchn_fifo_vcpu);
     if ( !efv )
         return -ENOMEM;
 
-    rc = map_guest_page(d, gfn, &virt);
+    for ( i = 0; i <= EVTCHN_FIFO_PRIORITY_MIN; i++ )
+        init_queue(v, &efv->queue[i], i);
+
+    v->evtchn_fifo = efv;
+
+    return 0;
+}
+
+static int map_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
+{
+    void *virt;
+    unsigned int i;
+    int rc;
+
+    if ( v->evtchn_fifo->control_block )
+        return -EINVAL;
+
+    rc = map_guest_page(v->domain, gfn, &virt);
     if ( rc < 0 )
-    {
-        xfree(efv);
         return rc;
-    }
 
-    v->evtchn_fifo = efv;
     v->evtchn_fifo->control_block = virt + offset;
 
     for ( i = 0; i <= EVTCHN_FIFO_PRIORITY_MIN; i++ )
-        init_queue(v, &v->evtchn_fifo->queue[i], i);
+        v->evtchn_fifo->queue[i].head = &v->evtchn_fifo->control_block->head[i];
 
     return 0;
 }
@@ -508,28 +527,43 @@ int evtchn_fifo_init_control(struct evtc
 
     spin_lock(&d->event_lock);
 
-    rc = setup_control_block(v, gfn, offset);
-
     /*
      * If this is the first control block, setup an empty event array
      * and switch to the fifo port ops.
      */
-    if ( rc == 0 && !d->evtchn_fifo )
+    if ( !d->evtchn_fifo )
     {
+        struct vcpu *vcb;
+
+        for_each_vcpu ( d, vcb ) {
+            rc = setup_control_block(vcb);
+            if ( rc < 0 )
+                goto error;
+        }
+
         rc = setup_event_array(d);
         if ( rc < 0 )
-            cleanup_control_block(v);
-        else
-        {
-            d->evtchn_port_ops = &evtchn_port_ops_fifo;
-            d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
-            setup_ports(d);
-        }
+            goto error;
+
+        rc = map_control_block(v, gfn, offset);
+        if ( rc < 0 )
+            goto error;
+
+        d->evtchn_port_ops = &evtchn_port_ops_fifo;
+        d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
+        setup_ports(d);
     }
+    else
+        rc = map_control_block(v, gfn, offset);
 
     spin_unlock(&d->event_lock);
 
     return rc;
+
+ error:
+    evtchn_fifo_destroy(d);
+    spin_unlock(&d->event_lock);
+    return rc;
 }
 
 static int add_page_to_event_array(struct domain *d, unsigned long gfn)

evtchn: check control block exists when using FIFO-based events

When using the FIFO-based event channels, there are no checks for the
existance of a control block when binding an event or moving it to a
different VCPU.  This is because events may be bound when the ABI is
in 2-level mode (e.g., by the toolstack before the domain is started).

The guest may trigger a Xen crash in evtchn_fifo_set_pending() if:

  a) the event is bound to a VCPU without a control block; or
  b) VCPU 0 does not have a control block.

In case (a), Xen will crash when looking up the current queue.  In
(b), Xen will crash when looking up the old queue (which defaults to a
queue on VCPU 0).

By allocating all the per-VCPU structures when enabling the FIFO ABI,
we can be sure that v->evtchn_fifo is always valid.

EVTCHNOP_init_control_block for all the other CPUs need only map the
shared control block.

A single check in evtchn_fifo_set_pending() before accessing the
control block fixes all cases where the guest has not initialized some
control blocks.

This is XSA-107.

Reported-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: David Vrabel <[email protected]>
Reviewed-by: Jan Beulich <[email protected]>

--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -178,6 +178,18 @@ static void evtchn_fifo_set_pending(stru
         bool_t linked = 0;
 
         /*
+         * Control block not mapped.  The guest must not unmask an
+         * event until the control block is initialized, so we can
+         * just drop the event.
+         */
+        if ( unlikely(!v->evtchn_fifo->control_block) )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pv has no FIFO event channel control block\n", v);
+            goto done;
+        }
+
+        /*
          * No locking around getting the queue. This may race with
          * changing the priority but we are allowed to signal the
          * event once on the old priority.
@@ -385,36 +397,42 @@ static void init_queue(struct vcpu *v, s
 {
     spin_lock_init(&q->lock);
     q->priority = i;
-    q->head = &v->evtchn_fifo->control_block->head[i];
 }
 
-static int setup_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
+static int setup_control_block(struct vcpu *v)
 {
-    struct domain *d = v->domain;
     struct evtchn_fifo_vcpu *efv;
-    void *virt;
     unsigned int i;
-    int rc;
-
-    if ( v->evtchn_fifo )
-        return -EINVAL;
 
     efv = xzalloc(struct evtchn_fifo_vcpu);
     if ( !efv )
         return -ENOMEM;
 
-    rc = map_guest_page(d, gfn, &virt);
+    for ( i = 0; i <= EVTCHN_FIFO_PRIORITY_MIN; i++ )
+        init_queue(v, &efv->queue[i], i);
+
+    v->evtchn_fifo = efv;
+
+    return 0;
+}
+
+static int map_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
+{
+    void *virt;
+    unsigned int i;
+    int rc;
+
+    if ( v->evtchn_fifo->control_block )
+        return -EINVAL;
+
+    rc = map_guest_page(v->domain, gfn, &virt);
     if ( rc < 0 )
-    {
-        xfree(efv);
         return rc;
-    }
 
-    v->evtchn_fifo = efv;
     v->evtchn_fifo->control_block = virt + offset;
 
     for ( i = 0; i <= EVTCHN_FIFO_PRIORITY_MIN; i++ )
-        init_queue(v, &v->evtchn_fifo->queue[i], i);
+        v->evtchn_fifo->queue[i].head = &v->evtchn_fifo->control_block->head[i];
 
     return 0;
 }
@@ -509,28 +527,43 @@ int evtchn_fifo_init_control(struct evtc
 
     spin_lock(&d->event_lock);
 
-    rc = setup_control_block(v, gfn, offset);
-
     /*
      * If this is the first control block, setup an empty event array
      * and switch to the fifo port ops.
      */
-    if ( rc == 0 && !d->evtchn_fifo )
+    if ( !d->evtchn_fifo )
     {
+        struct vcpu *vcb;
+
+        for_each_vcpu ( d, vcb ) {
+            rc = setup_control_block(vcb);
+            if ( rc < 0 )
+                goto error;
+        }
+
         rc = setup_event_array(d);
         if ( rc < 0 )
-            cleanup_control_block(v);
-        else
-        {
-            d->evtchn_port_ops = &evtchn_port_ops_fifo;
-            d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
-            setup_ports(d);
-        }
+            goto error;
+
+        rc = map_control_block(v, gfn, offset);
+        if ( rc < 0 )
+            goto error;
+
+        d->evtchn_port_ops = &evtchn_port_ops_fifo;
+        d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
+        setup_ports(d);
     }
+    else
+        rc = map_control_block(v, gfn, offset);
 
     spin_unlock(&d->event_lock);
 
     return rc;
+
+ error:
+    evtchn_fifo_destroy(d);
+    spin_unlock(&d->event_lock);
+    return rc;
 }
 
 static int add_page_to_event_array(struct domain *d, unsigned long gfn)