Re: use-after-free in usbnet
From: Huajun Li <hidden>
Date: 2012-04-21 06:39:20
On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei [off-list ref] wrote:
On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li [off-list ref] wrote:quoted
On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei [off-list ref] wrote:quoted
On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li [off-list ref] wrote:quoted
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?unlink_urbs() complete handler -------------------------------------- ------------------------------------------------- spin_unlock_irqrestore() rx_complete() derver_bh() __skb_unlink() __skb_queue_tail(&dev->done, skb) =======> skb is moved to dev->done, and can be freed by usbnet_bh() skb_queue_walk_safe() tmp = skb->next ===> refer to freed skbI see the problem, so looks skb_queue_walk_safe is not safe. I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe is needed and it may become unsafe if skb is freed during current loop. But removing the 2nd 'tmp = skb->next' doesn't help the problem, because tmp still may become freed after releasing lock.quoted
If its state is x_done/tx_done/rx_cleanup, that means the the skb will be released soon, right? If so, it should avoid calling usb_unlink_urb().Even though you can avoid calling unlink for completed URBs, the skbs still may be freed in unlink path because complete handler will be triggered by unlink and the referenced skb may be freed before next loop, so your patch can't fix the oops.
Hi Ming,
That's why my patch uses skb_queue_walk() to traverse the queue,
it guarantees the skb available in each loop. Is this what you
expected?
The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
1. If the skb in txq/rxq, then it must be available,
unlink_urbs() can refer to it safely while it holds q->lock;
2. If the skb in txq/rxq and its state is
rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
don't need to unlink it again;
3. Before releasing q->lock in unlink_urbs(), it will increase
the URB's refercount, so even the related skb is freed in future, the
URB is still available.
Thanks,
--Huajun
As far as I can think of, we can hold lock of done queue to forbid skb free during unlinking. The below patch may fix the problem, are you OK with it?
Just skip trying this per your following email's comments.
quoted hunk
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index db99536..a9809d4 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c@@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, structsk_buff_head *q) struct sk_buff *skb, *skbnext; int count = 0; - spin_lock_irqsave (&q->lock, flags); + spin_lock_irqsave(&dev->done.lock, flags); + spin_lock(&q->lock); skb_queue_walk_safe(q, skb, skbnext) { struct skb_data *entry; struct urb *urb;@@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, structsk_buff_head *q) * handler(include defer_bh). */ usb_get_urb(urb); - spin_unlock_irqrestore(&q->lock, flags); + spin_unlock(&q->lock); // during some PM-driven resume scenarios, // these (async) unlinks complete immediately retval = usb_unlink_urb (urb);@@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,struct sk_buff_head *q) else count++; usb_put_urb(urb); - spin_lock_irqsave(&q->lock, flags); + spin_lock(&q->lock); } - spin_unlock_irqrestore (&q->lock, flags); + spin_unlock(&q->lock); + spin_unlock_irqrestore(&dev->done.lock, flags); return count; } 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