Re: use-after-free in usbnet

Hi Huajun,

On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li [off-list ref] wrote:

On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei [off-list ref] wrote:

quoted

Hi Huajun,

On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li [off-list ref] wrote:

quoted

Did we on the same page, could you please review my patch again?

My draft patch was based on current mainline( 3.4.0-rc3)  which had
already integrated your previous patch. And in my patch, it replaced
skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
= skb->next'  any more.

Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
the oops, does it?

No.
In each loop, my patch traverse the queue from its head, and it always
holds  q->lock when it need refer "skb->next", this can make sure the
right skb is not moved out of rxq/txq.

OK, your patch can avoid the oops, sorry for miss the point.

Can this fix what you concern? If so, IMO, there is no need to revert
your previous patch.

But your patch may introduce another problem, in fact, what your patch does
is basically same with the below change[1]:

So we can find easily that one same URB may be unlinked more than one
time with your patch because usb_unlink_urb is asynchronous even though
it behaves synchronously sometimes.

I remembered that is not allowed, at least usb_unlink_urb's comment says so:

          URBs complete only once per submission, and may be canceled only
          once per submission.


[1], against 3.4.0-rc3

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..aadf009 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c

@@ -578,15 +578,19 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
 static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
 {
 	unsigned long		flags;
-	struct sk_buff		*skb, *skbnext;
+	struct sk_buff		*skb;
 	int			count = 0;

 	spin_lock_irqsave (&q->lock, flags);
-	skb_queue_walk_safe(q, skb, skbnext) {
+	while (1) {
 		struct skb_data		*entry;
 		struct urb		*urb;
 		int			retval;

+		skb = q->next;
+		if (skb == (struct sk_buff *)q)
+			break;
+
 		entry = (struct skb_data *) skb->cb;
 		urb = entry->urb;

Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html