netdev - Fwd: usbnet: Recursive Locking bug ?

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-next>] [day] [month] [year] [list]

Date:	Tue, 7 Dec 2010 16:15:38 +0000
From:	Neil Jones <neiljay@...il.com>
To:	netdev@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Fwd: usbnet: Recursive Locking bug ?

> But what makes sure that the URB unlinked in unlink_urbs() stays a valid pointer?
> The bottom half may run and free the URB.

I think it may be safe to remove the lock as we are walking a list of
SKBs not URBs,
the BH can remove SKB's from the list but we are doing a safe list walk.
+ the BH takse the list lock when it does the remove.

I could be wrong though?


On Tue, Dec 7, 2010 at 2:54 PM, Oliver Neukum <oneukum@...e.de> wrote:
> Am Dienstag, 7. Dezember 2010, 15:08:43 schrieb Neil Jones:
>> I think this is due to unlink_urbs():
>
> Definitely.
>
>> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>> {
>>         unsigned long           flags;
>>         struct sk_buff          *skb, *skbnext;
>>         int                     count = 0;
>>
>>         spin_lock_irqsave (&q->lock, flags);
>>         skb_queue_walk_safe(q, skb, skbnext) {
>>                 struct skb_data         *entry;
>>                 struct urb              *urb;
>>                 int                     retval;
>>
>>                 entry = (struct skb_data *) skb->cb;
>>                 urb = entry->urb;
>>
>>                 // during some PM-driven resume scenarios,
>>                 // these (async) unlinks complete immediately
>>                 retval = usb_unlink_urb (urb);
>>                 if (retval != -EINPROGRESS && retval != 0)
>>                         netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>>                 else
>>                         count++;
>>         }
>>         spin_unlock_irqrestore (&q->lock, flags);
>>         return count;
>> }
>>
>> I dont think this should hold the list lock when calling usb_unlink_urb (urb).
>> The completion for the URBs takes the lock as required in its bottom half:
>
> But what makes sure that the URB unlinked in unlink_urbs() stays a valid pointer?
> The bottom half may run and free the URB.
>
>        Regards
>                Oliver
>
--
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