[RFC] EHCI: add to memory barrier to updating hw_next

Commit Message

Ming Lei July 19, 2013, 3:50 a.m. UTC

On Thu, Jul 18, 2013 at 10:08 PM, Alan Stern <[e-mail]> wrote:
> On Thu, 18 Jul 2013, Ming Lei wrote:
>
>> > I guess that HC could have a use-after-free problem like following situation.
>> >
>> > 1. A qtd which is not at the queue head should be removed in qh_completions().
>> > 2. The last->hw_next become be pointing at the next qtd but the hw_next value is delayed in write-buffer.
>> > 3. The qtd is removed in the list.
>> > 4. The qtd is freed into DMA pool and re-allocated for another urb.
>> > 5. HC try to process last->hw_next and it is pointing re-allocated qtd.
>> >
>> > What do you think about it? Is it possible?
>>
>> I understand it might not be possible because:  when 'stopped' is set, that
>> said the HC might not advance the queue. But I don't understand why
>> 'last->hw_next' is patched here under 'stopped' situation.
>
> It should not be possible.  When "stopped" is set, the QH gets unlinked
> and relinked before it can start up again.  Relinking involves some
> memory barriers, so the qTD will not be accessed again by the HC.
>
> last->hw_next gets patched because the qTD might belong to some URB in
> the middle of the queue that is being unlinked.  The URBs before it and
> after it will still be active, so the queue link has to be updated.

'stopped' is set under below situations:

     - unlink over or shutdown
     - halt
     - short packet(URB_SHORT_NOT_OK)

Looks EHCI won't advance the queue(qh) any more on above situations, so I
think last->hw_next might not need patching.

>
>> Even the 'stopped' case may be seldom triggered, do you know under
>> which condition the stopped is triggered in your problem?(stall, short read
>> or others)
>
> I was going to ask the same question.  This particular piece of code
> gets executed _only_ when an URB is unlinked.  Not during any other
> kind of error.

The code may run under 'halt' or short packet(URB_SHORT_NOT_OK) too.
If Gioh's problem falls to these two situations, below patch might be helpful.

Because the qh will be unlinked when there is fault in the endpoint(halt, short
packet), maybe it is safer to complete these URBs after the qh becomes
unlinked,  like what the blew patch does:



Thanks,

Patch

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index b637a65..6a65e0a 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -454,6 +454,10 @@  qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 			}
 		}

+		/* complete URBs after unlinking */
+		if (stopped && state != QH_STATE_IDLE)
+			goto exit;
+
 		/* unless we already know the urb's status, collect qtd status
 		 * and update count of bytes transferred.  in common short read
 		 * cases with only one data qtd (including control transfers),
@@ -489,15 +493,6 @@  qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 			}
 		}

-		/* if we're removing something not at the queue head,
-		 * patch the hardware queue pointer.
-		 */
-		if (stopped && qtd->qtd_list.prev != &qh->qtd_list) {
-			last = list_entry (qtd->qtd_list.prev,
-					struct ehci_qtd, qtd_list);
-			last->hw_next = qtd->hw_next;
-		}
-
 		/* remove qtd; it's recycled after possible urb completion */
 		list_del (&qtd->qtd_list);
 		last = qtd;
@@ -520,7 +515,7 @@  qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)

 		/* Otherwise the caller must unlink the QH. */
 	}
-
+ exit:
 	/* restore original state; caller must unlink or relink */
 	qh->qh_state = state;