Re: [PATCH 2/2] evtchn/fifo: don't corrupt queues if an old tail is linked

[Keir Fraser <keir.xen@gmail.com>]

On 10/12/2013 13:57, "David Vrabel" <david.vrabel@citrix.com> wrote:

> From: David Vrabel <david.vrabel@citrix.com>
> 
> An event may still be the tail of a queue even if the queue is now
> empty (an 'old tail' event).  There is logic to handle the case when
> this old tail event needs to be added to the now empty queue (by
> checking for q->tail == port).
> 
> However, this does not cover all cases.
> 
> 1. An old tail may be re-added simultaneously with another event.
>    LINKED is set on the old tail, and the other CPU may misinterpret
>    this as the old tail still being valid and set LINK instead of
>    HEAD.  All events on this queue will then be lost.
> 
> 2. If the old tail event on queue A is moved to a different queue B
>    (by changing its VCPU or priority), the event may then be linked
>    onto queue B.  When another event is linked onto queue A it will
>    check the old tail, see that it is linked (but on queue B) and
>    overwrite the LINK field, corrupting both queues.
> 
> When an event is linked, save the vcpu id and priority of the queue it
> is being linked onto.  Use this when linking an event to check if it
> is an unlinked old tail event.  If it is an old tail event, the old
> queue is empty and old_q->tail is invalidated to ensure adding another
> event to old_q will update HEAD.  The tail is invalidated by setting
> it to 0 since the event 0 is never linked.
> 
> The old_q->lock is held while setting LINKED to avoid the race with
> the test of LINKED in evtchn_fifo_set_link().
> 
> Since a event channel may move queues after old_q->lock is acquired,
> we must check that we have the correct lock and retry if not.  Since
> changing VCPUs or priority is expected to be rare events that are
> serialized in the guest, we try at most 3 times before dropping the
> event.  This prevents a malicious guest from repeatedly adjusting
> priority to prevent another domain from acquiring old_q->lock.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---

Acked-by: Keir Fraser <keir@xen.org>