Re: [ 174/218] Bluetooth: Fix use-after-free bug in SMP

From: Ben Hutchings
Date: Sat Sep 29 2012 - 12:30:28 EST

On Fri, 2012-09-28 at 13:16 -0700, Greg Kroah-Hartman wrote:
> 3.4-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Andre Guedes <andre.guedes@xxxxxxxxxxxxx>
>
> commit 61a0cfb008f57ecf7eb28ee762952fb42dc15d15 upstream.
>
> If SMP fails, we should always cancel security_timer delayed work.
> Otherwise, security_timer function may run after l2cap_conn object
> has been freed.
[...]

This particular bug doesn't appear to exist in earlier kernel versions,
but it led me to find some related bugs in teardown that do.

I'm attaching two patches:
- bluetooth-fix-l2cap_conn_del-locking.patch seems to be needed for both
3.0 and 3.2. It's very different from the upstream changes, and is
compile-tested only.
- bluetooth-fix-deadlock-and-crash-when-smp-pairing-times-out.patch
(commit d06cc416f517a25713dedd9e2a9ccf4f3086c09a upstream) seems to be
needed for 3.2 only.

What do you think?

Ben.

--
Ben Hutchings
Usenet is essentially a HUGE group of people passing notes in class.
- Rachel Kadel, `A Quick Guide to Newsgroup Etiquette'
From: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
Date: Sat, 29 Sep 2012 17:53:28 +0200
Subject: Bluetooth: Fix l2cap_conn_del() locking

This is covered by commit 3df91ea20e744344100b10ae69a17211fcf5b207
('Bluetooth: Revert to mutexes from RCU list') upstream.

l2cap_conn_del() can race with various other functions that walk the
channel list; in particular l2cap_conn_start() which is called from
l2cap_info_timeout(). l2cap_chan_del() itself takes the chan_lock for
writing, so instead of locking here we must move all channels onto a
temporary list.

Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
---
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -968,6 +968,7 @@ static void l2cap_info_timeout(unsigned
static void l2cap_conn_del(struct hci_conn *hcon, int err)
{
struct l2cap_conn *conn = hcon->l2cap_data;
+ struct list_head chan_l;
struct l2cap_chan *chan, *l;
struct sock *sk;

@@ -978,8 +979,13 @@ static void l2cap_conn_del(struct hci_co

kfree_skb(conn->rx_skb);

+ INIT_LIST_HEAD(&chan_l);
+ write_lock_bh(&conn->chan_lock);
+ list_splice_init(&conn->chan_l, &chan_l);
+ write_unlock_bh(&conn->chan_lock);
+
/* Kill channels */
- list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
+ list_for_each_entry_safe(chan, l, &chan_l, list) {
sk = chan->sk;
bh_lock_sock(sk);
l2cap_chan_del(chan, err);
From: Johan Hedberg <johan.hedberg@xxxxxxxxx>
Date: Wed, 6 Jun 2012 18:44:11 +0800
Subject: Bluetooth: Fix deadlock and crash when SMP pairing times out

commit d06cc416f517a25713dedd9e2a9ccf4f3086c09a upstream.

The l2cap_conn_del function tries to cancel_sync the security timer, but
when it's called from the timeout function itself a deadlock occurs.
Subsequently the "hcon->l2cap_data = NULL" that's supposed to protect
multiple calls to l2cap_conn_del never gets cleared and when the
connection finally drops we double free's etc which will crash the
kernel.

This patch fixes the issue by using the HCI_CONN_LE_SMP_PEND for
protecting against this. The same flag is also used for the same purpose
in other places in the SMP code.

Signed-off-by: Johan Hedberg <johan.hedberg@xxxxxxxxx>
Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
[bwh: Backported to 3.2: pending flags are in hci_conn::pend]
Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
---
net/bluetooth/l2cap_core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1009,7 +1009,12 @@ static void security_timeout(unsigned lo
{
struct l2cap_conn *conn = (void *) arg;

- l2cap_conn_del(conn->hcon, ETIMEDOUT);
+ BT_DBG("conn %p", conn);
+
+ if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->pend)) {
+ smp_chan_destroy(conn);
+ l2cap_conn_del(conn->hcon, ETIMEDOUT);
+ }
}

static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)

Attachment: signature.asc
Description: This is a digitally signed message part