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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@xxxxxxxxxx> 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.

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel