evtchn: avoid NULL derefs

Commit fbbd5009e6 ("evtchn: refactor low-level event channel port ops")
added a de-reference of the struct evtchn pointer for a port without
first making sure the bucket pointer is non-NULL. This de-reference is
actually entirely unnecessary, as all relevant callers (beyond the
problematic do_poll()) already hold the port number in their hands, and
the actual leaf functions need nothing else.

For FIFO event channels there's a second problem in that the ordering
of reads and updates to ->num_evtchns and ->event_array[] was so far
undefined (the read side isn't always holding the domain's event lock).
Add respective barriers.

This is XSA-221.

Reported-by: Ankur Arora <ankur.a.arora@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
master commit: e7719a0dfa
master date: 2017-06-20 14:37:47 +0200

xen/arch/x86/irq.c

@@ -1489,7 +1489,7 @@ int pirq_guest_unmask(struct domain *d)
         {
             pirq = pirqs[i]->pirq;
             if ( pirqs[i]->masked &&
-                 !evtchn_port_is_masked(d, evtchn_from_port(d, pirqs[i]->evtchn)) )
+                 !evtchn_port_is_masked(d, pirqs[i]->evtchn) )
                 pirq_guest_eoi(pirqs[i]);
         }
     } while ( ++pirq < d->nr_pirqs && n == ARRAY_SIZE(pirqs) );
@@ -2247,7 +2247,6 @@ static void dump_irqs(unsigned char key)
     int i, irq, pirq;
     struct irq_desc *desc;
     irq_guest_action_t *action;
-    struct evtchn *evtchn;
     struct domain *d;
     const struct pirq *info;
     unsigned long flags;
@@ -2290,11 +2289,10 @@ static void dump_irqs(unsigned char key)
                 d = action->guest[i];
                 pirq = domain_irq_to_pirq(d, irq);
                 info = pirq_info(d, pirq);
-                evtchn = evtchn_from_port(d, info->evtchn);
                 printk("%u:%3d(%c%c%c)",
                        d->domain_id, pirq,
-                       (evtchn_port_is_pending(d, evtchn) ? 'P' : '-'),
-                       (evtchn_port_is_masked(d, evtchn) ? 'M' : '-'),
+                       evtchn_port_is_pending(d, info->evtchn) ? 'P' : '-',
+                       evtchn_port_is_masked(d, info->evtchn) ? 'M' : '-',
                        (info->masked ? 'M' : '-'));
                 if ( i != action->nr_guests )
                     printk(",");

xen/common/event_2l.c

@@ -62,16 +62,20 @@ static void evtchn_2l_unmask(struct domain *d, struct evtchn *evtchn)
     }
 }
 
-static bool_t evtchn_2l_is_pending(struct domain *d,
-                                   const struct evtchn *evtchn)
+static bool_t evtchn_2l_is_pending(struct domain *d, evtchn_port_t port)
 {
-    return test_bit(evtchn->port, &shared_info(d, evtchn_pending));
+    unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
+
+    ASSERT(port < max_ports);
+    return port < max_ports && test_bit(port, &shared_info(d, evtchn_pending));
 }
 
-static bool_t evtchn_2l_is_masked(struct domain *d,
-                                  const struct evtchn *evtchn)
+static bool_t evtchn_2l_is_masked(struct domain *d, evtchn_port_t port)
 {
-    return test_bit(evtchn->port, &shared_info(d, evtchn_mask));
+    unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
+
+    ASSERT(port < max_ports);
+    return port >= max_ports || test_bit(port, &shared_info(d, evtchn_mask));
 }
 
 static void evtchn_2l_print_state(struct domain *d,

xen/common/event_channel.c

@@ -1381,8 +1381,8 @@ static void domain_dump_evtchn_info(struct domain *d)
 
         printk("    %4u [%d/%d/",
                port,
-               !!evtchn_port_is_pending(d, chn),
-               !!evtchn_port_is_masked(d, chn));
+               evtchn_port_is_pending(d, port),
+               evtchn_port_is_masked(d, port));
         evtchn_port_print_state(d, chn);
         printk("]: s=%d n=%d x=%d",
                chn->state, chn->notify_vcpu_id, chn->xen_consumer);

xen/common/event_fifo.c

@@ -28,6 +28,12 @@ static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d,
     if ( unlikely(port >= d->evtchn_fifo->num_evtchns) )
         return NULL;
 
+    /*
+     * Callers aren't required to hold d->event_lock, so we need to synchronize
+     * with add_page_to_event_array().
+     */
+    smp_rmb();
+
     p = port / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
     w = port % EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
 
@@ -288,24 +294,22 @@ static void evtchn_fifo_unmask(struct domain *d, struct evtchn *evtchn)
         evtchn_fifo_set_pending(v, evtchn);
 }
 
-static bool_t evtchn_fifo_is_pending(struct domain *d,
-                                     const struct evtchn *evtchn)
+static bool_t evtchn_fifo_is_pending(struct domain *d, evtchn_port_t port)
 {
     event_word_t *word;
 
-    word = evtchn_fifo_word_from_port(d, evtchn->port);
+    word = evtchn_fifo_word_from_port(d, port);
     if ( unlikely(!word) )
         return 0;
 
     return test_bit(EVTCHN_FIFO_PENDING, word);
 }
 
-static bool_t evtchn_fifo_is_masked(struct domain *d,
-                                    const struct evtchn *evtchn)
+static bool_t evtchn_fifo_is_masked(struct domain *d, evtchn_port_t port)
 {
     event_word_t *word;
 
-    word = evtchn_fifo_word_from_port(d, evtchn->port);
+    word = evtchn_fifo_word_from_port(d, port);
     if ( unlikely(!word) )
         return 1;
 
@@ -594,6 +598,10 @@ static int add_page_to_event_array(struct domain *d, unsigned long gfn)
         return rc;
 
     d->evtchn_fifo->event_array[slot] = virt;
+
+    /* Synchronize with evtchn_fifo_word_from_port(). */
+    smp_wmb();
+
     d->evtchn_fifo->num_evtchns += EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
 
     /*

xen/common/schedule.c

@@ -955,7 +955,7 @@ static long do_poll(struct sched_poll *sched_poll)
             goto out;
 
         rc = 0;
-        if ( evtchn_port_is_pending(d, evtchn_from_port(d, port)) )
+        if ( evtchn_port_is_pending(d, port) )
             goto out;
     }
 

xen/include/xen/event.h

@@ -137,8 +137,8 @@ struct evtchn_port_ops {
     void (*set_pending)(struct vcpu *v, struct evtchn *evtchn);
     void (*clear_pending)(struct domain *d, struct evtchn *evtchn);
     void (*unmask)(struct domain *d, struct evtchn *evtchn);
-    bool_t (*is_pending)(struct domain *d, const struct evtchn *evtchn);
-    bool_t (*is_masked)(struct domain *d, const struct evtchn *evtchn);
+    bool_t (*is_pending)(struct domain *d, evtchn_port_t port);
+    bool_t (*is_masked)(struct domain *d, evtchn_port_t port);
     /*
      * Is the port unavailable because it's still being cleaned up
      * after being closed?
@@ -175,15 +175,15 @@ static inline void evtchn_port_unmask(struct domain *d,
 }
 
 static inline bool_t evtchn_port_is_pending(struct domain *d,
-                                            const struct evtchn *evtchn)
+                                            evtchn_port_t port)
 {
-    return d->evtchn_port_ops->is_pending(d, evtchn);
+    return d->evtchn_port_ops->is_pending(d, port);
 }
 
 static inline bool_t evtchn_port_is_masked(struct domain *d,
-                                           const struct evtchn *evtchn)
+                                           evtchn_port_t port)
 {
-    return d->evtchn_port_ops->is_masked(d, evtchn);
+    return d->evtchn_port_ops->is_masked(d, port);
 }
 
 static inline bool_t evtchn_port_is_busy(struct domain *d, evtchn_port_t port)