linux-kernel - hci_usb: races, races, races

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

Date:	Mon, 31 Mar 2008 22:18:33 +0200
From:	Pavel Machek <pavel@....cz>
To:	marcel@...tmann.org, maxk@...lcomm.com,
	kernel list <linux-kernel@...r.kernel.org>, oliver@...kum.name
Subject: hci_usb: races, races, races

Hi!

hci_usb interacts badly with system suspend... it corrupts memory
after it was freed. To test, just run ppp-over-bluetooth, do some data
transfers, and s2ram. Unplugging hci_usb would probably kill it as well.

@@ -447,11 +449,20 @@ static int hci_usb_close(struct hci_dev 
 
        BT_DBG("%s", hdev->name);
 
        /* Synchronize with completion handlers */
        write_lock_irqsave(&husb->completion_lock, flags);
        write_unlock_irqrestore(&husb->completion_lock, flags);
 
-       hci_usb_unlink_urbs(husb);
+//     hci_usb_unlink_urbs(husb);
        hci_usb_flush(hdev);
        return 0;
 }


Now... hci_usb_unlink_urbs is running in paralel with completions,
freeing memory.

Unfortunately, hci_usb_tx_complete touches that memory:

@@ -735,14 +748,15 @@ static void hci_usb_tx_complete(struct u
        BT_DBG("%s urb %p status %d flags %x", hdev->name, urb,
                        urb->status, urb->transfer_flags);
 
+       /* _urb may have been already cleared by hci_usb_unlink_urbs
*/
        atomic_dec(__pending_tx(husb, _urb->type));
 
        urb->transfer_buffer = NULL;
        kfree_skb((struct sk_buff *) _urb->priv);
 
       if (!test_bit(HCI_RUNNING, &hdev->flags))
               return;

        if (!urb->status)
                hdev->stat.byte_tx += urb->transfer_buffer_length;
        else


HCI_RUNNING test is too late... and anyway, USB stack was touching
that memory while we were freeing it. I don't know about simple fix.

Workaround is:

@@ -447,11 +449,20 @@ static int hci_usb_close(struct hci_dev

	BT_DBG("%s", hdev->name);

	/* Synchronize with completion handlers */
	write_lock_irqsave(&husb->completion_lock, flags);
	write_unlock_irqrestore(&husb->completion_lock, flags);

-       hci_usb_unlink_urbs(husb);
+//     hci_usb_unlink_urbs(husb);
	hci_usb_flush(hdev);
        return 0;
 }

hci_usb.h is racy, too:

 _urb->queue = q is used for locking, without barriers etc... this
could fix it, but it is still ugly. If BUG() never triggers (it really
should not) we should remove the test, and we'll get non-ugly solution.

hci_usb.h
@@ -75,7 +70,7 @@ static inline void _urb_queue_head(struc
 {
        unsigned long flags;
        spin_lock_irqsave(&q->lock, flags);
-       list_add(&_urb->list, &q->head); _urb->queue = q;
+       _urb->queue = q; mb(); list_add(&_urb->list, &q->head);
        spin_unlock_irqrestore(&q->lock, flags);
 }
 
@@ -83,19 +78,32 @@ static inline void _urb_queue_tail(struc
 {
        unsigned long flags;
        spin_lock_irqsave(&q->lock, flags);
-       list_add_tail(&_urb->list, &q->head); _urb->queue = q;
+       /* Hmm.... we only update _urb->queue _after_ putting it on
+        * list. Someone may see the 0 value... and urb_unlink just
+        * silently does nothing when it sees NULL. */
+       _urb->queue = q; mb(); list_add_tail(&_urb->list, &q->head);
        spin_unlock_irqrestore(&q->lock, flags);
 }
 
 static inline void _urb_unlink(struct _urb *_urb)
 {
-       struct _urb_queue *q = _urb->queue;
+       struct _urb_queue *q;
        unsigned long flags;
+       /* This is at least strange. If we rely on _urb->queue for
locking
+          we should test it _inside_ some lock.
+
+          In combination with no locking in urb_queue_tail, that
+          means that we may return with urb still on the list.
+       */
+
+       mb();
+       q = _urb->queue;
+
        if (q) {
                spin_lock_irqsave(&q->lock, flags);
                list_del(&_urb->list); _urb->queue = NULL;
                spin_unlock_irqrestore(&q->lock, flags);
-       }
+       } else  BUG();
 }
 
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Download attachment "bt.1.mesg.gz" of type "application/octet-stream" (28279 bytes)

Download attachment "bt.2.mesg.gz" of type "application/octet-stream" (23556 bytes)

Download attachment "bt.3.mesg.gz" of type "application/octet-stream" (14924 bytes)