From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 3/3] evtchn/fifo: don't spin indefinitely when setting LINK Date: Mon, 4 Nov 2013 14:52:50 +0000 Message-ID: <5277B4C2.5000508@citrix.com> References: <1383231791-4604-1-git-send-email-david.vrabel@citrix.com> <1383231791-4604-4-git-send-email-david.vrabel@citrix.com> <5277BF9F02000078000FF1F2@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VdLWR-0000cB-9R for xen-devel@lists.xenproject.org; Mon, 04 Nov 2013 14:52:55 +0000 In-Reply-To: <5277BF9F02000078000FF1F2@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 04/11/13 14:39, Jan Beulich wrote: >>>> On 31.10.13 at 16:03, David Vrabel wrote: >> From: David Vrabel >> >> 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