thr3ads.net - Xen devel - [PATCHv2 0/3] Xen: FIFO-based event channel ABI fixes [Oct 2013]

If this information is useful, please help other people find it:
Share via:

David Vrabel

2013-Oct-31 15:03 UTC

[PATCHv2 0/3] Xen: FIFO-based event channel ABI fixes

This series addresses two design flaws in the FIFO-based event channel
ABI.

1. Allow guests to expand the array after binding events.

2. Fix a potential DoS caused by an unbounded loop when setting LINK.

An updated design document is available from:

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

- Add section on memory barriers.
- EVTCHNOP expand array should be called after binding events.
- Guest clearing MASKED on a tail event is no longer valid, a
  hypercall is required. This allows the loop in the Xen''s link()
  function to be bounded.

v8 of the Linux patches will be posted shortly.

Changes since v1:
- Add MAINTAINERS patch
- Remove some unnecessary temporary pending state clears
- Add fix for DoS

David Vrabel

2013-Oct-31 15:03 UTC

head link

[PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

From: David Vrabel <dvrabel@cantab.net>

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 MAINTAINERS |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index adacac2..d7825c0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -156,6 +156,12 @@ F:     xen/include/efi/
 F:     xen/include/asm-x86/efi*.h
 F:     xen/include/asm-x86/x86_*/efi*.h
 
+EVENT CHANNELS (FIFO-BASED ABI)
+M:	David Vrabel <david.vrabel@citrix.com>
+S:	Supported
+F:	xen/common/event_fifo.c
+F:	xen/include/xen/event_fifo.h
+
 GDBSX DEBUGGER
 M:	Mukesh Rathor <mukesh.rathor@oracle.com>
 S:	Supported
-- 
1.7.2.5

David Vrabel

2013-Oct-31 15:03 UTC

head link

[PATCH 2/3] evtchn: don''t lose pending state if FIFO event array page is missing

From: David Vrabel <david.vrabel@citrix.com>

When the FIFO-based ABI is in use, if an event is bound when the
corresponding event array page is missing any attempt to set the event
pending will lose the event (because there is nowhere to write the
pending state).

This wasn''t initially considered an issue because guests were expected
to only bind events once they had expanded the event array, however:

1. A domain may start with events already bound (by the toolstack).

2. The guest does not know what the port number will be until the
   event is bound (it doesn''t know how many already bound events there
   are), so it does not know how many event array pages are required.
   This makes it difficult to expand in advanced (the current Linux
   implementation expands after binding for example).

To prevent pending events from being lost because there is no array
page, temporarily store the pending state in evtchn->pending.  When an
array page is added, use this state to set the port as pending.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/event_fifo.c |   47 +++++++++++++++++++++++++++++++++++++++++------
 xen/include/xen/sched.h |    1 +
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index bec8d87..64dbfff 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -61,8 +61,16 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct
evtchn *evtchn)
 
     port = evtchn->port;
     word = evtchn_fifo_word_from_port(d, port);
+
+    /*
+     * Event array page may not exist yet, save the pending state for
+     * when the page is added.
+     */
     if ( unlikely(!word) )
+    {
+        evtchn->pending = 1;
         return;
+    }
 
     /*
      * No locking around getting the queue. This may race with
@@ -322,16 +330,29 @@ static void cleanup_event_array(struct domain *d)
     xfree(d->evtchn_fifo);
 }
 
-static void set_priority_all(struct domain *d, unsigned int priority)
+static void setup_ports(struct domain *d)
 {
     unsigned int port;
 
+    /*
+     * For each port that is already bound:
+     *
+     * - save its pending state.
+     * - set default priority.
+     */
     for ( port = 1; port < d->max_evtchns; port++ )
     {
+        struct evtchn *evtchn;
+
         if ( !port_is_valid(d, port) )
             break;
 
-        evtchn_port_set_priority(d, evtchn_from_port(d, port), priority);
+        evtchn = evtchn_from_port(d, port);
+
+        if ( test_bit(port, &shared_info(d, evtchn_pending)) )
+            evtchn->pending = 1;
+
+        evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
     }
 }
 
@@ -369,9 +390,6 @@ int evtchn_fifo_init_control(struct evtchn_init_control
*init_control)
     /*
      * If this is the first control block, setup an empty event array
      * and switch to the fifo port ops.
-     *
-     * Any ports currently bound will have their priority set to the
-     * default.
      */
     if ( rc == 0 && !d->evtchn_fifo )
     {
@@ -382,7 +400,7 @@ int evtchn_fifo_init_control(struct evtchn_init_control
*init_control)
         {
             d->evtchn_port_ops = &evtchn_port_ops_fifo;
             d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
-            set_priority_all(d, EVTCHN_FIFO_PRIORITY_DEFAULT);
+            setup_ports(d);
         }
     }
 
@@ -395,6 +413,7 @@ static int add_page_to_event_array(struct domain *d,
unsigned long gfn)
 {
     void *virt;
     unsigned int slot;
+    unsigned int port = d->evtchn_fifo->num_evtchns;
     int rc;
 
     slot = d->evtchn_fifo->num_evtchns /
EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
@@ -408,6 +427,22 @@ static int add_page_to_event_array(struct domain *d,
unsigned long gfn)
     d->evtchn_fifo->event_array[slot] = virt;
     d->evtchn_fifo->num_evtchns += EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
 
+    /*
+     * Re-raise any events that were pending while this array page was
+     * missing.
+     */
+    for ( ; port < d->evtchn_fifo->num_evtchns; port++ )
+    {
+        struct evtchn *evtchn;
+
+        if ( !port_is_valid(d, port) )
+            break;
+
+        evtchn = evtchn_from_port(d, port);
+        if ( evtchn->pending )
+            evtchn_fifo_set_pending(d->vcpu[evtchn->notify_vcpu_id],
evtchn);
+    }
+
     return 0;
 }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 25bf637..624ea15 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -97,6 +97,7 @@ struct evtchn
         u16 virq;      /* state == ECS_VIRQ */
     } u;
     u8 priority;
+    u8 pending:1;
 #ifdef FLASK_ENABLE
     void *ssid;
 #endif
-- 
1.7.2.5

David Vrabel

2013-Oct-31 15:03 UTC

head link

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

From: David Vrabel <david.vrabel@citrix.com>

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@citrix.com>
---
 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

David Vrabel

2013-Oct-31 15:13 UTC

head link

Re: [PATCHv2 0/3] Xen: FIFO-based event channel ABI fixes

Sorry Jan, wrong email address.

On 31/10/2013 15:03, David Vrabel wrote:> This series addresses two design flaws in the FIFO-based event channel
> ABI.
> 
> 1. Allow guests to expand the array after binding events.
> 
> 2. Fix a potential DoS caused by an unbounded loop when setting LINK.
> 
> An updated design document is available from:
> 
> http://xenbits.xen.org/people/dvrabel/event-channels-G.pdf
> 
> - Add section on memory barriers.
> - EVTCHNOP expand array should be called after binding events.
> - Guest clearing MASKED on a tail event is no longer valid, a
>   hypercall is required. This allows the loop in the Xen''s link()
>   function to be bounded.
> 
> v8 of the Linux patches will be posted shortly.
> 
> Changes since v1:
> - Add MAINTAINERS patch
> - Remove some unnecessary temporary pending state clears
> - Add fix for DoS
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Boris Ostrovsky

2013-Oct-31 18:13 UTC

head link

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

On 10/31/2013 11:03 AM, David Vrabel wrote:> From: David Vrabel <david.vrabel@citrix.com>
>
> 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@citrix.com>
> ---
>   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;
Failure to set link here is an unexpected outcome, possibly indicating a 
malicious guest. Should you emit a (rate-limited) warning for this case?

-boris
>   }
> @@ -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

Jan Beulich

2013-Nov-04 14:29 UTC

head link

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@citrix.com>
wrote:
> From: David Vrabel <dvrabel@cantab.net>
> 
Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  MAINTAINERS |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index adacac2..d7825c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -156,6 +156,12 @@ F:     xen/include/efi/
>  F:     xen/include/asm-x86/efi*.h
>  F:     xen/include/asm-x86/x86_*/efi*.h
>  
> +EVENT CHANNELS (FIFO-BASED ABI)
> +M:	David Vrabel <david.vrabel@citrix.com>
> +S:	Supported
> +F:	xen/common/event_fifo.c
> +F:	xen/include/xen/event_fifo.h
> +
>  GDBSX DEBUGGER
>  M:	Mukesh Rathor <mukesh.rathor@oracle.com>
>  S:	Supported
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Jan Beulich

2013-Nov-04 14:29 UTC

head link

Re: [PATCH 2/3] evtchn: don''t lose pending state if FIFO event array page is missing

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@citrix.com>
wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> When the FIFO-based ABI is in use, if an event is bound when the
> corresponding event array page is missing any attempt to set the event
> pending will lose the event (because there is nowhere to write the
> pending state).
> 
> This wasn''t initially considered an issue because guests were
expected
> to only bind events once they had expanded the event array, however:
> 
> 1. A domain may start with events already bound (by the toolstack).
> 
> 2. The guest does not know what the port number will be until the
>    event is bound (it doesn''t know how many already bound events
there
>    are), so it does not know how many event array pages are required.
>    This makes it difficult to expand in advanced (the current Linux
>    implementation expands after binding for example).
> 
> To prevent pending events from being lost because there is no array
> page, temporarily store the pending state in evtchn->pending.  When an
> array page is added, use this state to set the port as pending.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/common/event_fifo.c |   47
+++++++++++++++++++++++++++++++++++++++++------
>  xen/include/xen/sched.h |    1 +
>  2 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index bec8d87..64dbfff 100644
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -61,8 +61,16 @@ static void evtchn_fifo_set_pending(struct vcpu *v,
struct
> evtchn *evtchn)
>  
>      port = evtchn->port;
>      word = evtchn_fifo_word_from_port(d, port);
> +
> +    /*
> +     * Event array page may not exist yet, save the pending state for
> +     * when the page is added.
> +     */
>      if ( unlikely(!word) )
> +    {
> +        evtchn->pending = 1;
>          return;
> +    }
>  
>      /*
>       * No locking around getting the queue. This may race with
> @@ -322,16 +330,29 @@ static void cleanup_event_array(struct domain *d)
>      xfree(d->evtchn_fifo);
>  }
>  
> -static void set_priority_all(struct domain *d, unsigned int priority)
> +static void setup_ports(struct domain *d)
>  {
>      unsigned int port;
>  
> +    /*
> +     * For each port that is already bound:
> +     *
> +     * - save its pending state.
> +     * - set default priority.
> +     */
>      for ( port = 1; port < d->max_evtchns; port++ )
>      {
> +        struct evtchn *evtchn;
> +
>          if ( !port_is_valid(d, port) )
>              break;
>  
> -        evtchn_port_set_priority(d, evtchn_from_port(d, port), priority);
> +        evtchn = evtchn_from_port(d, port);
> +
> +        if ( test_bit(port, &shared_info(d, evtchn_pending)) )
> +            evtchn->pending = 1;
> +
> +        evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
>      }
>  }
>  
> @@ -369,9 +390,6 @@ int evtchn_fifo_init_control(struct evtchn_init_control
> *init_control)
>      /*
>       * If this is the first control block, setup an empty event array
>       * and switch to the fifo port ops.
> -     *
> -     * Any ports currently bound will have their priority set to the
> -     * default.
>       */
>      if ( rc == 0 && !d->evtchn_fifo )
>      {
> @@ -382,7 +400,7 @@ int evtchn_fifo_init_control(struct evtchn_init_control
> *init_control)
>          {
>              d->evtchn_port_ops = &evtchn_port_ops_fifo;
>              d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
> -            set_priority_all(d, EVTCHN_FIFO_PRIORITY_DEFAULT);
> +            setup_ports(d);
>          }
>      }
>  
> @@ -395,6 +413,7 @@ static int add_page_to_event_array(struct domain *d, 
> unsigned long gfn)
>  {
>      void *virt;
>      unsigned int slot;
> +    unsigned int port = d->evtchn_fifo->num_evtchns;
>      int rc;
>  
>      slot = d->evtchn_fifo->num_evtchns /
EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
> @@ -408,6 +427,22 @@ static int add_page_to_event_array(struct domain *d, 
> unsigned long gfn)
>      d->evtchn_fifo->event_array[slot] = virt;
>      d->evtchn_fifo->num_evtchns += EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
>  
> +    /*
> +     * Re-raise any events that were pending while this array page was
> +     * missing.
> +     */
> +    for ( ; port < d->evtchn_fifo->num_evtchns; port++ )
> +    {
> +        struct evtchn *evtchn;
> +
> +        if ( !port_is_valid(d, port) )
> +            break;
> +
> +        evtchn = evtchn_from_port(d, port);
> +        if ( evtchn->pending )
> +            evtchn_fifo_set_pending(d->vcpu[evtchn->notify_vcpu_id],
evtchn);
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 25bf637..624ea15 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -97,6 +97,7 @@ struct evtchn
>          u16 virq;      /* state == ECS_VIRQ */
>      } u;
>      u8 priority;
> +    u8 pending:1;
>  #ifdef FLASK_ENABLE
>      void *ssid;
>  #endif
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Jan Beulich

2013-Nov-04 14:39 UTC

head link

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

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@citrix.com>
wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> 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.
So here you talk of two guests (with me not immediately seeing
where that interaction comes from - is it that for an interdomain
event the receiver could harm the sender?), ...
> 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.
... yet here it is not really clear which guest the last "guest"
refers
to (i.e. it''s fine if the malicious guest harms itself, but the change
would be pointless if the malicious guest could still harm the other
one).
> @@ -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);
>  
>          /*
I fail to see how this change is related to the rest of the patch.

Jan

David Vrabel

2013-Nov-04 14:52 UTC

head link

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

On 04/11/13 14:39, Jan Beulich wrote:>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> 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.
> 
> So here you talk of two guests (with me not immediately seeing
> where that interaction comes from - is it that for an interdomain
> event the receiver could harm the sender?), ...
Yes.  Guest A notifies guest M which requires linking a new event into
one of guest B''s event queue.  While guest A is writing the guest
M''s
event array (to set the LINK field), guest M may repeatedly write to the
same event word, causing the cmpxchg() to repeatedly fail.

Guest A could also be the hypervisor for VIRQs and other events raised
by Xen.
>> 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.
> 
> ... yet here it is not really clear which guest the last "guest"
refers
> to (i.e. it''s fine if the malicious guest harms itself, but the
change
> would be pointless if the malicious guest could still harm the other
> one).
The malicious guest loses the event.
>> @@ -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);
>>  
>>          /*
> 
> I fail to see how this change is related to the rest of the patch.
This is needed so the correct queue lock is used in evtchn_fifo_unmask().

David

Jan Beulich

2013-Nov-04 14:57 UTC

head link

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

>>> On 04.11.13 at 15:52, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 04/11/13 14:39, Jan Beulich wrote:
>>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>>> 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.
>> 
>> ... yet here it is not really clear which guest the last
"guest" refers
>> to (i.e. it''s fine if the malicious guest harms itself, but
the change
>> would be pointless if the malicious guest could still harm the other
>> one).
> 
> The malicious guest loses the event.
So then "that guest" would perhaps be more precise than "the
guest"?
>>> @@ -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);
>>>  
>>>          /*
>> 
>> I fail to see how this change is related to the rest of the patch.
> 
> This is needed so the correct queue lock is used in evtchn_fifo_unmask().
I guessed that, but I wasn''t able to immediately see why that would
not have been a requirement before.

Jan

Ian Campbell

2013-Nov-04 15:07 UTC

head link

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

On Mon, 2013-11-04 at 14:52 +0000, David Vrabel wrote:> On 04/11/13 14:39, Jan Beulich wrote:
> >>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> 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.
> > 
> > So here you talk of two guests (with me not immediately seeing
> > where that interaction comes from - is it that for an interdomain
> > event the receiver could harm the sender?), ...
> 
> Yes.  Guest A notifies guest M which requires linking a new event into
> one of guest B''s event queue.  While guest A is writing the guest
M''s
> event array (to set the LINK field), guest M may repeatedly write to the
> same event word, causing the cmpxchg() to repeatedly fail.
M == B here?

Ian.

David Vrabel

2013-Nov-04 15:11 UTC

head link

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

On 04/11/13 15:07, Ian Campbell wrote:> On Mon, 2013-11-04 at 14:52 +0000, David Vrabel wrote:
>> On 04/11/13 14:39, Jan Beulich wrote:
>>>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>
>>>> 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.
>>>
>>> So here you talk of two guests (with me not immediately seeing
>>> where that interaction comes from - is it that for an interdomain
>>> event the receiver could harm the sender?), ...
>>
>> Yes.  Guest A notifies guest M which requires linking a new event into
>> one of guest B''s event queue.  While guest A is writing the
guest M''s
>> event array (to set the LINK field), guest M may repeatedly write to
the
>> same event word, causing the cmpxchg() to repeatedly fail.
> 
> M == B here?
Yes. I originally had B then changed it to M for Malicious to be clearer...

David

David Vrabel

2013-Nov-04 16:30 UTC

head link

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

On 04/11/13 14:57, Jan Beulich wrote:>>>> On 04.11.13 at 15:52, David Vrabel
<david.vrabel@citrix.com> wrote:
>> On 04/11/13 14:39, Jan Beulich wrote:
>>>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>>>> 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.
>>>
>>> ... yet here it is not really clear which guest the last
"guest" refers
>>> to (i.e. it''s fine if the malicious guest harms itself,
but the change
>>> would be pointless if the malicious guest could still harm the
other
>>> one).
>>
>> The malicious guest loses the event.
> 
> So then "that guest" would perhaps be more precise than "the
> guest"?
Yes.  I''ll improve the commit message to be clearer.
>>>> @@ -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);
>>>>  
>>>>          /*
>>>
>>> I fail to see how this change is related to the rest of the patch.
>>
>> This is needed so the correct queue lock is used in
evtchn_fifo_unmask().
> 
> I guessed that, but I wasn''t able to immediately see why that
would
> not have been a requirement before.
An event now has two queues associated with it.

1. The queue it is supposed to be on.  This is found with
evtchn->notify_vcpu_id and evtchn->priority when an event is being linked.

2. The queue the event is on right now (the new evtchn->q), until this
patch we didn''t care which queue an event was on, only that it was on
some queue.

These are not necessarily the same since an event may be moved between
VCPUs or have its priority changed while it is already on a queue.

Some additional notes on the locking:

The locking in the FIFO-based ABI is designed on the assumption that
every hypervisor operation on an event channel (set pending, unmask) is
done while holding a per-event channel spin lock.

Currently, the per-event locking is done with the coarser per-domain
event_lock.

The queue lock serializes adding events and clearing MASKED on tail events.

The event lock also protects evtchn->q and the evtchn->q->tail
=evtchn->port test.

David

Jan Beulich

2013-Nov-05 14:18 UTC

head link

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

>>> On 04.11.13 at 17:30, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 04/11/13 14:57, Jan Beulich wrote:
>>>>> On 04.11.13 at 15:52, David Vrabel
<david.vrabel@citrix.com> wrote:
>>> On 04/11/13 14:39, Jan Beulich wrote:
>>>>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>>>>>          event_word_t *tail_word;
>>>>>          bool_t linked = 0;
>>>>>  
>>>>> +        evtchn->q = q;
>>>>> +
>>>>>          spin_lock_irqsave(&q->lock, flags);
>>>>>  
>>>>>          /*
>>>>
>>>> I fail to see how this change is related to the rest of the
patch.
>>>
>>> This is needed so the correct queue lock is used in
evtchn_fifo_unmask().
>> 
>> I guessed that, but I wasn''t able to immediately see why that
would
>> not have been a requirement before.
> 
> An event now has two queues associated with it.
> 
> 1. The queue it is supposed to be on.  This is found with
> evtchn->notify_vcpu_id and evtchn->priority when an event is being
linked.
> 
> 2. The queue the event is on right now (the new evtchn->q), until this
> patch we didn''t care which queue an event was on, only that it was
on
> some queue.
> 
> These are not necessarily the same since an event may be moved between
> VCPUs or have its priority changed while it is already on a queue.
> 
> Some additional notes on the locking:
> 
> The locking in the FIFO-based ABI is designed on the assumption that
> every hypervisor operation on an event channel (set pending, unmask) is
> done while holding a per-event channel spin lock.
> 
> Currently, the per-event locking is done with the coarser per-domain
> event_lock.
> 
> The queue lock serializes adding events and clearing MASKED on tail events.
> 
> The event lock also protects evtchn->q and the evtchn->q->tail
=> evtchn->port test.
But aren''t you setting evtchn->q too late then?
evtchn_fifo_unmask() racing evtchn_fifo_set_pending() could
then lead to the latter seeing ->q still clear, the former setting it
and doing its linking work while in parallel the latter might clear
MASKED at any time.

Plus - shouldn''t you clear ->q at some point again?

Jan

Jan Beulich

2013-Nov-05 14:19 UTC

head link

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

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@citrix.com>
wrote:
> --- 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;
> +
This is superfluous in C, since ...
>  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. */
... this already puts it at global scope (other than in C++).

Jan

Jan Beulich

2013-Nov-05 14:25 UTC

head link

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

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@citrix.com>
wrote:
> @@ -98,6 +100,7 @@ struct evtchn
>      } u;
>      u8 priority;
>      u8 pending:1;
> +    struct evtchn_fifo_queue *q; /* Latest queue this event was on. */
Oh, and - just like for the original q you had here in an earlier
version of the FIFO patches - I think you could get away with
again just storing the "current" priority (8 bits) instead of the
queue pointer (8 bytes).

Jan

Keir Fraser

2013-Nov-05 21:06 UTC

head link

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

On 04/11/2013 14:29, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>> From: David Vrabel <dvrabel@cantab.net>
>> 
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>  MAINTAINERS |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index adacac2..d7825c0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -156,6 +156,12 @@ F:     xen/include/efi/
>>  F:     xen/include/asm-x86/efi*.h
>>  F:     xen/include/asm-x86/x86_*/efi*.h
>>  
>> +EVENT CHANNELS (FIFO-BASED ABI)
>> +M: David Vrabel <david.vrabel@citrix.com>
>> +S: Supported
>> +F: xen/common/event_fifo.c
>> +F: xen/include/xen/event_fifo.h
>> +
>>  GDBSX DEBUGGER
>>  M: Mukesh Rathor <mukesh.rathor@oracle.com>
>>  S: Supported
>> -- 
>> 1.7.2.5
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Keir Fraser

2013-Nov-05 21:07 UTC

head link

Re: [PATCH 2/3] evtchn: don''t lose pending state if FIFO event array page is missing

On 04/11/2013 14:29, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>> 
>> When the FIFO-based ABI is in use, if an event is bound when the
>> corresponding event array page is missing any attempt to set the event
>> pending will lose the event (because there is nowhere to write the
>> pending state).
>> 
>> This wasn''t initially considered an issue because guests were
expected
>> to only bind events once they had expanded the event array, however:
>> 
>> 1. A domain may start with events already bound (by the toolstack).
>> 
>> 2. The guest does not know what the port number will be until the
>>    event is bound (it doesn''t know how many already bound
events there
>>    are), so it does not know how many event array pages are required.
>>    This makes it difficult to expand in advanced (the current Linux
>>    implementation expands after binding for example).
>> 
>> To prevent pending events from being lost because there is no array
>> page, temporarily store the pending state in evtchn->pending.  When
an
>> array page is added, use this state to set the port as pending.
>> 
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>

David Vrabel

2013-Nov-06 11:49 UTC

head link

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

On 05/11/13 21:06, Keir Fraser wrote:> On 04/11/2013 14:29, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>>> From: David Vrabel <dvrabel@cantab.net>
>>>
>>
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Keir Fraser <keir@xen.org>
Thanks.

Jan, can you either fix up the author to use my citrix.com email or wait
for me to repost v3 of this series.

David

Jan Beulich

2013-Nov-06 12:40 UTC

head link

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

>>> On 06.11.13 at 12:49, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 05/11/13 21:06, Keir Fraser wrote:
>> On 04/11/2013 14:29, "Jan Beulich" <JBeulich@suse.com>
wrote:
>> 
>>>>>> On 31.10.13 at 16:03, David Vrabel
<david.vrabel@citrix.com> wrote:
>>>> From: David Vrabel <dvrabel@cantab.net>
>>>>
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Acked-by: Keir Fraser <keir@xen.org>
> 
> Thanks.
> 
> Jan, can you either fix up the author to use my citrix.com email or wait
> for me to repost v3 of this series.
I was about to say "too late", but git has authorship as you want
it (I''m generally purging the From: lines when they appear to match
the first Signed-off-by, as the result would be the same with my
commit script, and it saves me from accidentally leaving the From:
in the commit message).

Jan

David Vrabel

2013-Nov-06 13:38 UTC

head link

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

On 31/10/13 15:03, David Vrabel wrote:> From: David Vrabel <david.vrabel@citrix.com>
> 
> 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.
Given the non-obvious locking required for this to be safe and the
overhead of the guest having to do a unmask hypercall more often.  I
think I will fix this differently.

A new BUSY bit is added to the event word.  Xen sets BUSY prior to
updating the LINK field and then clears it when the LINK field is set.

When the guest unmasks an event it must spin waiting for BUSY to clear
before it clears the MASKED bit.  It then need only do the unmask
hypercall if the event is pending (as before).

Draft H and v3 of this series to follow.

David

Boris Ostrovsky

2013-Nov-06 15:01 UTC

head link

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

On 11/06/2013 08:38 AM, David Vrabel wrote:> On 31/10/13 15:03, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> 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.
> Given the non-obvious locking required for this to be safe and the
> overhead of the guest having to do a unmask hypercall more often.  I
> think I will fix this differently.
>
> A new BUSY bit is added to the event word.  Xen sets BUSY prior to
> updating the LINK field and then clears it when the LINK field is set.
>
> When the guest unmasks an event it must spin waiting for BUSY to clear
> before it clears the MASKED bit.  It then need only do the unmask
> hypercall if the event is pending (as before).
>
> Draft H and v3 of this series to follow.
Will there be a new Linux series as well?

-boris

David Vrabel

2013-Nov-06 15:07 UTC

head link

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

On 06/11/13 15:01, Boris Ostrovsky wrote:> On 11/06/2013 08:38 AM, David Vrabel wrote:
>> 
>> Given the non-obvious locking required for this to be safe and the
>> overhead of the guest having to do a unmask hypercall more often.  I
>> think I will fix this differently.
>>
>> A new BUSY bit is added to the event word.  Xen sets BUSY prior to
>> updating the LINK field and then clears it when the LINK field is set.
>>
>> When the guest unmasks an event it must spin waiting for BUSY to clear
>> before it clears the MASKED bit.  It then need only do the unmask
>> hypercall if the event is pending (as before).
>>
>> Draft H and v3 of this series to follow.
> 
> Will there be a new Linux series as well?
Yes, but only the final patch in the series will change.

David

Matt Wilson

2013-Nov-10 21:21 UTC

head link

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

On Thu, Oct 31, 2013 at 03:03:11PM +0000, David Vrabel
wrote:> From: David Vrabel <david.vrabel@citrix.com>
> 
> 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@citrix.com>
Wasn''t this

Reported-by: Anthony Liguori <aliguori@amazon.com>

at Xen Developer Summit?

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

Xen devel - Oct 2013 - [PATCHv2 0/3] Xen: FIFO-based event channel ABI fixes

[PATCHv2 0/3] Xen: FIFO-based event channel ABI fixes

[PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

[PATCH 2/3] evtchn: don''t lose pending state if FIFO event array page is missing

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

Re: [PATCHv2 0/3] Xen: FIFO-based event channel ABI fixes

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

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

Re: [PATCH 2/3] evtchn: don''t lose pending state if FIFO event array page is missing

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

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

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

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

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

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

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

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

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

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

Re: [PATCH 2/3] evtchn: don''t lose pending state if FIFO event array page is missing

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

Re: [PATCH 1/3] MAINTAINERS: Add FIFO-based event channel ABI maintainer

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

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

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

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