netdev - Re: use-after-free in usbnet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 21 Apr 2012 17:40:16 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Huajun Li <huajun.li.lee@...il.com>
Cc:	Oliver Neukum <oneukum@...e.de>,
	Alan Stern <stern@...land.harvard.edu>,
	Dave Jones <davej@...hat.com>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org,
	Fedora Kernel Team <kernel-team@...oraproject.org>
Subject: Re: use-after-free in usbnet

Hi Huajun,

On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
> On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming@...il.com> wrote:
>> Hi Huajun,
>>
>> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
>>
>>> 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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html