From: Jan Beulich Subject: 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 Signed-off-by: Jan Beulich --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1486,7 +1486,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) ); @@ -2244,7 +2244,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; @@ -2287,11 +2286,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(","); --- a/xen/common/event_2l.c +++ b/xen/common/event_2l.c @@ -61,16 +61,20 @@ static void evtchn_2l_unmask(struct doma } } -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, --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -1380,8 +1380,8 @@ static void domain_dump_evtchn_info(stru 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); --- a/xen/common/event_fifo.c +++ b/xen/common/event_fifo.c @@ -27,6 +27,12 @@ static inline event_word_t *evtchn_fifo_ 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; @@ -287,24 +293,22 @@ static void evtchn_fifo_unmask(struct do 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; @@ -593,6 +597,10 @@ static int add_page_to_event_array(struc 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; /* --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -965,7 +965,7 @@ static long do_poll(struct sched_poll *s goto out; rc = 0; - if ( evtchn_port_is_pending(d, evtchn_from_port(d, port)) ) + if ( evtchn_port_is_pending(d, port) ) goto out; } --- a/xen/include/xen/event.h +++ b/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(st } 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)