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

[Matt Wilson <msw@xxxxxxxxx>]

On Thu, Oct 31, 2013 at 03:03:11PM +0000, David Vrabel wrote:
> 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>

Wasn't this

Reported-by: Anthony Liguori <aliguori@xxxxxxxxxx>

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 mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel