[Xen-devel] [PATCH 3/3] evtchn/fifo: don't spin indefinitely when setting LINK

[David Vrabel <david.vrabel@xxxxxxxxxx>]

From: David Vrabel <david.vrabel@xxxxxxxxxx>

A malicious or buggy guest can cause another domain to spin
indefinitely by repeatedly writing to an event word when the other
domain is trying to link a new event.  The cmpxchg() in
evtchn_fifo_set_link() will repeatedly fail and the loop may never
terminate.

Fixing this requires a minor change to the ABI, which is documented in
draft G of the design.

http://xenbits.xen.org/people/dvrabel/event-channels-G.pdf

Since a well-behaved guest only makes a limited set of state changes,
the loop can terminate early if the guest makes an invalid state
transition.

The guest may:

- clear LINKED and link
- clear PENDING
- set MASKED
- clear MASKED

It is valid for the guest to mask and unmask an event at any time so
we specify that it is not valid for a guest to clear MASKED if the
event is the tail of a queue (i.e., LINKED is set and LINK is clear).
Instead, the guest must make an EVCHNOP_unmask hypercall to unmask the
event.

The hypercall ensures that UNMASKED isn't cleared on a tail event
whose LINK field is being set by holding the appropriate queue lock.

The remaining valid writes (clear LINKED, clear PENDING, set MASKED)
will limit the number of failures of the cmpxchg() to at most 3.  A
clear of LINKED will also terminate the loop early (as before).
Therefore, the loop can then be limited to at most 3 iterations.

If the buggy or malicious guest does cause the loop to exit early, the
newly pending event will be unreachable by the guest and it and
subsequent events may be lost.

Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
---
 xen/common/event_fifo.c |   19 +++++++++++++++++--
 xen/include/xen/sched.h |    3 +++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 64dbfff..6cf30ec 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -37,6 +37,7 @@ static inline event_word_t *evtchn_fifo_word_from_port(struct 
domain *d,
 static bool_t evtchn_fifo_set_link(event_word_t *word, uint32_t link)
 {
     event_word_t n, o, w;
+    unsigned int try = 3;
 
     w = *word;
 
@@ -45,7 +46,8 @@ static bool_t evtchn_fifo_set_link(event_word_t *word, 
uint32_t link)
             return 0;
         o = w;
         n = (w & ~EVTCHN_FIFO_LINK_MASK) | link;
-    } while ( (w = cmpxchg(word, o, n)) != o );
+        w = cmpxchg(word, o, n);
+    } while ( w != o && try-- );
 
     return 1;
 }
@@ -90,6 +92,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct 
evtchn *evtchn)
         event_word_t *tail_word;
         bool_t linked = 0;
 
+        evtchn->q = q;
+
         spin_lock_irqsave(&q->lock, flags);
 
         /*
@@ -148,7 +152,18 @@ static void evtchn_fifo_unmask(struct domain *d, struct 
evtchn *evtchn)
     if ( unlikely(!word) )
         return;
 
-    clear_bit(EVTCHN_FIFO_MASKED, word);
+    /*
+     * If this event is on the tail of a queue, the queue lock must be
+     * held to avoid clearing MASKED while setting LINK.
+     */
+    if ( evtchn->q && evtchn->q->tail == evtchn->port )
+    {
+        spin_lock_irq(&evtchn->q->lock);
+        clear_bit(EVTCHN_FIFO_MASKED, word);
+        spin_unlock_irq(&evtchn->q->lock);
+    }
+    else
+        clear_bit(EVTCHN_FIFO_MASKED, word);
 
     /* Relink if pending. */
     if ( test_bit(EVTCHN_FIFO_PENDING, word) )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 624ea15..7b0273a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -68,6 +68,8 @@ extern struct domain *dom0;
 #define EVTCHNS_PER_GROUP  (BUCKETS_PER_GROUP * EVTCHNS_PER_BUCKET)
 #define NR_EVTCHN_GROUPS   DIV_ROUND_UP(MAX_NR_EVTCHNS, EVTCHNS_PER_GROUP)
 
+struct evtchn_fifo_queue;
+
 struct evtchn
 {
 #define ECS_FREE         0 /* Channel is available for use.                  */
@@ -98,6 +100,7 @@ struct evtchn
     } u;
     u8 priority;
     u8 pending:1;
+    struct evtchn_fifo_queue *q; /* Latest queue this event was on. */
 #ifdef FLASK_ENABLE
     void *ssid;
 #endif
-- 
1.7.2.5


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel