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

[David Vrabel <david.vrabel@citrix.com>]

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