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

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

On 12/11/13 11:38, 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
> guest is trying to link a new event.  The cmpxchg() in
> evtchn_fifo_set_link() will repeatedly fail and the loop may never
> terminate.

This was fixed by introducing the BUSY bit (see below), but I wonder if
another solution would be better.

The MASKED bits could all be moved into a separate bit array, guest
writes to set/clear the MASKED bit would never conflict with Xen
updating the main event word.  The cmpxchg() loop is then trivially
bounded as the only valid writes by the guest are clear PENDING and
clear LINKED & LINK.

The masked array could be either:

1. In the same page as the event array.  This would waste a big chunk of
the event array page though, Doubling the memory requirements.

2. Have a separate set of pages.  EVTCHNOP_expand_array would be
extended to supply the GFN for the array page where necessary. For 2^17
events, 4 additional pages would be required for this masked array.

Thoughts?

David

> Fixing this requires a change to the ABI which is documented in draft
> H of the design.
> 
>   http://xenbits.xen.org/people/dvrabel/event-channels-H.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
> specify that it is not valid for a guest to clear MASKED if Xen is
> trying to update LINK.  Indicate this to the guest with an additional
> BUSY bit in the event word.  The guest must not clear MASKED if BUSY
> is set and it should spin until BUSY is cleared.
> 
> The remaining valid writes (clear LINKED, clear PENDING, set MASKED,
> clear MASKED by Xen) will limit the number of failures of the
> cmpxchg() to at most 4.  A clear of LINKED will also terminate the
> loop early. Therefore, the loop can then be limited to at most 4
> iterations.
> 
> If the buggy or malicious guest does cause the loop to exit with
> LINKED set and LINK unset then that buggy guest will lose events.