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] [day] [month] [year] [list]
Date:	Fri, 27 Apr 2012 00:30:49 +0800
From:	Huajun Li <huajun.li.lee@...il.com>
To:	Ming Lei <tom.leiming@...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

On Thu, Apr 26, 2012 at 1:02 PM, Ming Lei <tom.leiming@...il.com> wrote:
> Hi Huajun,
>
> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@...il.com> 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.
>>
>> 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.
>>
>> 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)
>> +                               continue;
>> +                       else
>> +                               break;
>> +               }
>> +
>> +               if (skb == (struct sk_buff *)(q))
>> +                       break;
>> +
>>                urb = entry->urb;
>>
>>
>
> After thinking about the issue further, the basic idea of your patch is good,
> but not safe(double unlink, also races on accessing entry->state), so I write
> a new one based on your patch.
>
> Are you OK this new one?
>

Yes,  no more issue that I can see by now, thanks.

> Thanks,
> --
> Ming Lei
>
> From 89eb41968caa9523c90c1cf7b7e2259db00e04b8 Mon Sep 17 00:00:00 2001
> From: Ming Lei <tom.leiming@...il.com>
> Date: Thu, 26 Apr 2012 11:33:46 +0800
> Subject: [PATCH] usbnet: fix skb traversing races during unlink
>
> Commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d(net/usbnet: avoid
> recursive locking in usbnet_stop()) fixes the recursive locking
> problem by releasing the skb queue lock before unlink, but may
> cause skb traversing races:
>        - after URB is unlinked and the queue lock is released,
>        the refered skb and skb->next may be moved to done queue,
>        even be released
>        - in skb_queue_walk_safe, the next skb is still obtained
>        by next pointer of the last skb
>        - so maybe trigger oops or other problems
>
> This patch extends the usage of entry->state to describe 'start_unlink'
> state, so always holding the queue(rx/tx) lock to change the state if
> the referd skb is in rx or tx queue because we need to know if the
> refered urb has been started unlinking in unlink_urbs.
>
> Also the patch uses usb_block_urb introduced by Oliver to block
> URB resubmit in complete handler if the URB will be unlinked.
>
> The other part of this patch is based on Hugjun's patch:

s/Hugjun/Huajun

> always traverse from head of the tx/rx queue to get skb which is
> to be unlinked but not been started unlinking.
> ---
--
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