From: Johan Hedberg <[email protected]>
The tk_request function takes the local IO capability as the second last
parameter and the remote IO capability as the first parameter. They were
previously swapped: when we receive a pairing response
req->io_capability contains the local one and rsp->io_capability the
remote one.
Signed-off-by: Johan Hedberg <[email protected]>
---
net/bluetooth/smp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index ff4835b..1885697 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -649,7 +649,7 @@ static u8 smp_cmd_pairing_rsp(struct l2cap_conn *conn, struct sk_buff *skb)
auth |= (req->auth_req | rsp->auth_req) & SMP_AUTH_MITM;
- ret = tk_request(conn, 0, auth, rsp->io_capability, req->io_capability);
+ ret = tk_request(conn, 0, auth, req->io_capability, rsp->io_capability);
if (ret)
return SMP_UNSPECIFIED;
--
1.7.10
Hi Johan,
* Johan Hedberg <[email protected]> [2012-06-06 18:44:11 +0800]:
> From: Johan Hedberg <[email protected]>
>
> 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 <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
It looks ok to me, patch has been applied to bluetooth.git. Thanks.
Gustavo
Hi Johan,
* Johan Hedberg <[email protected]> [2012-06-06 18:54:15 +0800]:
> From: Johan Hedberg <[email protected]>
>
> The tk_request function takes the local IO capability as the second last
> parameter and the remote IO capability as the last parameter. They were
> previously swapped: when we receive a pairing response
> req->io_capability contains the local one and rsp->io_capability the
> remote one.
>
> Signed-off-by: Johan Hedberg <[email protected]>
> ---
> v2: s/first/last/ in the commit message.
>
> net/bluetooth/smp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Patch has been applied to bluetooth.git, thanks.
Gustavo
Hi Johan,
On Thu, Jun 07, 2012 at 01:03:06AM +0800, Johan Hedberg wrote:
> Hi Vinicius,
>
> On Wed, Jun 06, 2012, Vinicius Costa Gomes wrote:
> > > 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.
> >
> > I wonder if (inside l2cap_conn_del()) we move "hcon->l2cap_data = NULL"
> > up in the function, probably next to the check for "!conn", would be a
> > safer alternative.
>
> That was one of the things I tried first as well (and it did remove the
> crash) but it doesn't remove the deadlock. The l2cap_conn_del would
> still deadlock in cancel_sync(sec_timer) when called from
> security_timeout().
Have you checked approach with refcnt I sent as RFC? It overcomes those
problems using similar techniques like for l2cap_chan delayed works.
> Moving the NULL assignment to the top would certainly help decrease the
> chance of calling l2cap_conn_del twice but it wouldn't completely remove
> this race condition as clearing the variable + testing its value isn't
> a single atomic operation.
>
> So I'd still consider this deadlock removal patch by itself as necessary
> and valid but additionally the race of calling l2cap_conn_del twice
> would need to be fixed with a separate patch, possibly involving some
> new or existing lock.
I will send updated RFC today. It works for BREDR, it would be good to
test it with LE.
Best regards
Andrei Emeltchenko
Hi Vinicius,
On Wed, Jun 06, 2012, Vinicius Costa Gomes wrote:
> > 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.
>
> I wonder if (inside l2cap_conn_del()) we move "hcon->l2cap_data = NULL"
> up in the function, probably next to the check for "!conn", would be a
> safer alternative.
That was one of the things I tried first as well (and it did remove the
crash) but it doesn't remove the deadlock. The l2cap_conn_del would
still deadlock in cancel_sync(sec_timer) when called from
security_timeout().
Moving the NULL assignment to the top would certainly help decrease the
chance of calling l2cap_conn_del twice but it wouldn't completely remove
this race condition as clearing the variable + testing its value isn't
a single atomic operation.
So I'd still consider this deadlock removal patch by itself as necessary
and valid but additionally the race of calling l2cap_conn_del twice
would need to be fixed with a separate patch, possibly involving some
new or existing lock.
Johan
Hi Johan,
On 18:44 Wed 06 Jun, Johan Hedberg wrote:
> From: Johan Hedberg <[email protected]>
>
> 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.
I wonder if (inside l2cap_conn_del()) we move "hcon->l2cap_data = NULL"
up in the function, probably next to the check for "!conn", would be a
safer alternative.
>
> 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 <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index f9bffe3..4ca8824 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1314,7 +1314,12 @@ static void security_timeout(struct work_struct *work)
> struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> security_timer.work);
>
> - l2cap_conn_del(conn->hcon, ETIMEDOUT);
> + BT_DBG("conn %p", conn);
> +
> + if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
> + smp_chan_destroy(conn);
> + l2cap_conn_del(conn->hcon, ETIMEDOUT);
> + }
> }
>
> static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
Hi Johan,
> The tk_request function takes the local IO capability as the second last
> parameter and the remote IO capability as the last parameter. They were
> previously swapped: when we receive a pairing response
> req->io_capability contains the local one and rsp->io_capability the
> remote one.
>
> Signed-off-by: Johan Hedberg <[email protected]>
> ---
> v2: s/first/last/ in the commit message.
>
> net/bluetooth/smp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
From: Johan Hedberg <[email protected]>
The tk_request function takes the local IO capability as the second last
parameter and the remote IO capability as the last parameter. They were
previously swapped: when we receive a pairing response
req->io_capability contains the local one and rsp->io_capability the
remote one.
Signed-off-by: Johan Hedberg <[email protected]>
---
v2: s/first/last/ in the commit message.
net/bluetooth/smp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index ff4835b..1885697 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -649,7 +649,7 @@ static u8 smp_cmd_pairing_rsp(struct l2cap_conn *conn, struct sk_buff *skb)
auth |= (req->auth_req | rsp->auth_req) & SMP_AUTH_MITM;
- ret = tk_request(conn, 0, auth, rsp->io_capability, req->io_capability);
+ ret = tk_request(conn, 0, auth, req->io_capability, rsp->io_capability);
if (ret)
return SMP_UNSPECIFIED;
--
1.7.10
From: Johan Hedberg <[email protected]>
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 <[email protected]>
---
net/bluetooth/l2cap_core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f9bffe3..4ca8824 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1314,7 +1314,12 @@ static void security_timeout(struct work_struct *work)
struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
security_timer.work);
- l2cap_conn_del(conn->hcon, ETIMEDOUT);
+ BT_DBG("conn %p", conn);
+
+ if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
+ smp_chan_destroy(conn);
+ l2cap_conn_del(conn->hcon, ETIMEDOUT);
+ }
}
static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
--
1.7.10