[PATCH 1/2] Bluetooth: Fix SMP pairing method selection

[PATCH 1/2] Bluetooth: Fix SMP pairing method selection

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

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 <johan.hedberg@intel.com>
---
 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

[PATCH 2/2] Bluetooth: Fix deadlock and crash when SMP pairing times out

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

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@intel.com>
---
 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

[PATCH 1/2 v2] Bluetooth: Fix SMP pairing method selection

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

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 <johan.hedberg@intel.com>
---
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

Re: [PATCH 1/2 v2] Bluetooth: Fix SMP pairing method selection

[Marcel Holtmann]

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 <johan.hedberg@intel.com>
> ---
> v2: s/first/last/ in the commit message.
> 
>  net/bluetooth/smp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Fix deadlock and crash when SMP pairing times out

[Vinicius Costa Gomes]

Hi Johan,

On 18:44 Wed 06 Jun, Johan Hedberg wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> 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 <johan.hedberg@intel.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
-- 
Vinicius

Re: [PATCH 2/2] Bluetooth: Fix deadlock and crash when SMP pairing times out

[Johan Hedberg]

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

Re: [PATCH 2/2] Bluetooth: Fix deadlock and crash when SMP pairing times out

[Andrei Emeltchenko]

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 

Re: [PATCH 1/2 v2] Bluetooth: Fix SMP pairing method selection

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2012-06-06 18:54:15 +0800]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> 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 <johan.hedberg@intel.com>
> ---
> 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

Re: [PATCH 2/2] Bluetooth: Fix deadlock and crash when SMP pairing times out

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2012-06-06 18:44:11 +0800]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> 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@intel.com>
> ---
>  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