Re: use-after-free in usbnet
From: Ming Lei <tom.leiming@gmail.com>
Date: 2012-04-20 14:23:00
Possibly related (same subject, not in this thread)
- 2012-04-26 · Re: use-after-free in usbnet · Huajun Li <hidden>
- 2012-04-26 · Re: use-after-free in usbnet · Ming Lei <hidden>
- 2012-04-22 · Re: use-after-free in usbnet · Ming Lei <tom.leiming@gmail.com>
- 2012-04-22 · Re: use-after-free in usbnet · Ming Lei <hidden>
- 2012-04-22 · Re: use-after-free in usbnet · Ming Lei <hidden>
On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li [off-list ref] wrote:
Above patch has already been integrated to mainline. However, maybe there still exists another potentail use-after-free issue, here is a case: After release the lock in unlink_urbs(), defer_bh() may move current skb from rxq/txq to dev->done queue, even cause the skb be released. Then in next loop cycle, it can't refer to expected skb, and may Oops again.
Could you explain in a bit detail? Why can't the expected skb be refered to in next loop?
To easily reproduce it, in unlink_urbs(), you can delay a short time after usb_put_urb(urb), then disconnect your device while transferring data, and repeat it times you will find errors on your screen.
Could you post out the error log?
quoted hunk ↗ jump to hunk
Following is a draft patch to guarantee the queue consistent, and refer to expected skb in each loop cycle:diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index b7b3f5b..6da0141 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c@@ -578,16 +578,28 @@ 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 (!skb_queue_empty(q)) { struct skb_data *entry; struct urb *urb; int retval; - entry = (struct skb_data *) skb->cb; + skb_queue_walk(q, skb) { + entry = (struct skb_data *)skb->cb; + if (entry->state == rx_done || + entry->state == tx_done || + entry->state == rx_cleanup)
Maybe it is not necessary, if the state has been changed to rx_done or tx_done or rx_cleanup, it means the URB referenced to by the skb has been completed, and the coming usb_unlink_urb can handle the case correctly.
+ continue; + else + break; + } + + if (skb == (struct sk_buff *)(q)) + break; + urb = entry->urb; Thanks, --Huajun Li
Thanks, -- Ming Lei