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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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