[PATCHv3 0/3] Bluetooth: L2CAP robustness fixes

[PATCHv3 0/3] Bluetooth: L2CAP robustness fixes

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

Fixes increase L2CAP robustness to DOS attacks to BT stack.

Andrei Emeltchenko (3):
  Bluetooth: fix MTU L2CAP configuration parameter
  Bluetooth: check for l2cap header in start fragment
  Bluetooth: check L2CAP length in first ACL fragment

 net/bluetooth/l2cap.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

[PATCHv3 1/3] Bluetooth: fix MTU L2CAP configuration parameter

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

When receiving L2CAP negative configuration response with respect
to MTU parameter we modify wrong field. MTU here means proposed
value of MTU that the remote device intends to transmit. So for local
L2CAP socket it is pi->imtu.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Acked-by: Ville Tervo <ville.tervo@nokia.com>
Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index c784703..9fad312 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2771,10 +2771,10 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
 		case L2CAP_CONF_MTU:
 			if (val < L2CAP_DEFAULT_MIN_MTU) {
 				*result = L2CAP_CONF_UNACCEPT;
-				pi->omtu = L2CAP_DEFAULT_MIN_MTU;
+				pi->imtu = L2CAP_DEFAULT_MIN_MTU;
 			} else
-				pi->omtu = val;
-			l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, pi->omtu);
+				pi->imtu = val;
+			l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, pi->imtu);
 			break;
 
 		case L2CAP_CONF_FLUSH_TO:
-- 
1.7.0.4

[PATCHv3 2/3] Bluetooth: check for l2cap header in start fragment

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

BLUETOOTH SPECIFICATION Version 4.0 [Vol 3] page 36 mentioned
"Note: Start Fragments always begin with the Basic L2CAP header
of a PDU."

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 net/bluetooth/l2cap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9fad312..ce8f5e4 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -4664,7 +4664,8 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
 			l2cap_conn_unreliable(conn, ECOMM);
 		}
 
-		if (skb->len < 2) {
+		/* Start fragment always begin with Basic L2CAP header */
+		if (skb->len < L2CAP_HDR_SIZE) {
 			BT_ERR("Frame is too short (len %d)", skb->len);
 			l2cap_conn_unreliable(conn, ECOMM);
 			goto drop;
-- 
1.7.0.4

[PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

Current Bluetooth code assembles fragments of big L2CAP packets
in l2cap_recv_acldata and then checks allowed L2CAP size in
assemled L2CAP packet (pi->imtu < skb->len).

The patch moves allowed L2CAP size check to the early stage when
we receive the first fragment of L2CAP packet. We do not need to
reserve and keep L2CAP fragments for bad packets.

Updated version after comments from Mat Martineau <mathewm@codeaurora.org>

Trace below is received when using stress tools sending big
fragmented L2CAP packets.
...
[ 1712.798492] swapper: page allocation failure. order:4, mode:0x4020
[ 1712.804809] [<c0031870>] (unwind_backtrace+0x0/0xdc) from [<c00a1f70>]
(__alloc_pages_nodemask+0x4)
[ 1712.814666] [<c00a1f70>] (__alloc_pages_nodemask+0x47c/0x4d4) from
[<c00a1fd8>] (__get_free_pages+)
[ 1712.824645] [<c00a1fd8>] (__get_free_pages+0x10/0x3c) from [<c026eb5c>]
(__alloc_skb+0x4c/0xfc)
[ 1712.833465] [<c026eb5c>] (__alloc_skb+0x4c/0xfc) from [<bf28c738>]
(l2cap_recv_acldata+0xf0/0x1f8 )
[ 1712.843322] [<bf28c738>] (l2cap_recv_acldata+0xf0/0x1f8 [l2cap]) from
[<bf0094ac>] (hci_rx_task+0x)
...

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 net/bluetooth/l2cap.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ce8f5e4..c95fbd8 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -4654,6 +4654,8 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
 
 	if (flags & ACL_START) {
 		struct l2cap_hdr *hdr;
+		struct sock *sk;
+		u16 cid;
 		int len;
 
 		if (conn->rx_len) {
@@ -4673,6 +4675,7 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
 
 		hdr = (struct l2cap_hdr *) skb->data;
 		len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
+		cid = __le16_to_cpu(hdr->cid);
 
 		if (len == skb->len) {
 			/* Complete frame received */
@@ -4689,6 +4692,20 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
 			goto drop;
 		}
 
+		sk = l2cap_get_chan_by_scid(&conn->chan_list, cid);
+
+		if (sk && l2cap_pi(sk)->imtu < len) {
+			BT_ERR("Frame exceeding recv MTU (len %d, MTU %d)",
+					len, l2cap_pi(sk)->imtu);
+			conn->rx_len = 0; /* needed? */
+			bh_unlock_sock(sk);
+			l2cap_conn_unreliable(conn, ECOMM);
+			goto drop;
+		}
+
+		if (sk)
+			bh_unlock_sock(sk);
+
 		/* Allocate skb for the complete frame (with header) */
 		conn->rx_skb = bt_skb_alloc(len, GFP_ATOMIC);
 		if (!conn->rx_skb)
-- 
1.7.0.4

Re: [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes

[Andrei Emeltchenko]

ping

On Thu, Sep 9, 2010 at 10:42 AM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> Fixes increase L2CAP robustness to DOS attacks to BT stack.
>
> Andrei Emeltchenko (3):
>  Bluetooth: fix MTU L2CAP configuration parameter
>  Bluetooth: check for l2cap header in start fragment
>  Bluetooth: check L2CAP length in first ACL fragment
>
>  net/bluetooth/l2cap.c |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
>
> --
> 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
>

Re: [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment

[Gustavo F. Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-09-09 10:43:01 +0300]:

> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> 
> Current Bluetooth code assembles fragments of big L2CAP packets
> in l2cap_recv_acldata and then checks allowed L2CAP size in
> assemled L2CAP packet (pi->imtu < skb->len).
> 
> The patch moves allowed L2CAP size check to the early stage when
> we receive the first fragment of L2CAP packet. We do not need to
> reserve and keep L2CAP fragments for bad packets.
> 
> Updated version after comments from Mat Martineau <mathewm@codeaurora.org>
> 
> Trace below is received when using stress tools sending big
> fragmented L2CAP packets.
> ...
> [ 1712.798492] swapper: page allocation failure. order:4, mode:0x4020
> [ 1712.804809] [<c0031870>] (unwind_backtrace+0x0/0xdc) from [<c00a1f70>]
> (__alloc_pages_nodemask+0x4)
> [ 1712.814666] [<c00a1f70>] (__alloc_pages_nodemask+0x47c/0x4d4) from
> [<c00a1fd8>] (__get_free_pages+)
> [ 1712.824645] [<c00a1fd8>] (__get_free_pages+0x10/0x3c) from [<c026eb5c>]
> (__alloc_skb+0x4c/0xfc)
> [ 1712.833465] [<c026eb5c>] (__alloc_skb+0x4c/0xfc) from [<bf28c738>]
> (l2cap_recv_acldata+0xf0/0x1f8 )
> [ 1712.843322] [<bf28c738>] (l2cap_recv_acldata+0xf0/0x1f8 [l2cap]) from
> [<bf0094ac>] (hci_rx_task+0x)
> ...
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
>  net/bluetooth/l2cap.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index ce8f5e4..c95fbd8 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -4654,6 +4654,8 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>  
>  	if (flags & ACL_START) {
>  		struct l2cap_hdr *hdr;
> +		struct sock *sk;
> +		u16 cid;
>  		int len;
>  
>  		if (conn->rx_len) {
> @@ -4673,6 +4675,7 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>  
>  		hdr = (struct l2cap_hdr *) skb->data;
>  		len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
> +		cid = __le16_to_cpu(hdr->cid);
>  
>  		if (len == skb->len) {
>  			/* Complete frame received */
> @@ -4689,6 +4692,20 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>  			goto drop;
>  		}
>  
> +		sk = l2cap_get_chan_by_scid(&conn->chan_list, cid);
> +
> +		if (sk && l2cap_pi(sk)->imtu < len) {
> +			BT_ERR("Frame exceeding recv MTU (len %d, MTU %d)",
> +					len, l2cap_pi(sk)->imtu);

I think you have to check if the imtu is less than (len -
L2CAP_HDR_SIZE) because we don't count the header in the MTU.

> +			conn->rx_len = 0; /* needed? */

It's not needed. rx_len is always zero here.


-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment

[Andrei Emeltchenko]

Hi Gustavo,

On Tue, Sep 14, 2010 at 9:21 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-09-09 10:43:01 +0300]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> Current Bluetooth code assembles fragments of big L2CAP packets
>> in l2cap_recv_acldata and then checks allowed L2CAP size in
>> assemled L2CAP packet (pi->imtu < skb->len).
>>
>> The patch moves allowed L2CAP size check to the early stage when
>> we receive the first fragment of L2CAP packet. We do not need to
>> reserve and keep L2CAP fragments for bad packets.
>>
>> Updated version after comments from Mat Martineau <mathewm@codeaurora.org>
>>
>> Trace below is received when using stress tools sending big
>> fragmented L2CAP packets.
>> ...
>> [ 1712.798492] swapper: page allocation failure. order:4, mode:0x4020
>> [ 1712.804809] [<c0031870>] (unwind_backtrace+0x0/0xdc) from [<c00a1f70>]
>> (__alloc_pages_nodemask+0x4)
>> [ 1712.814666] [<c00a1f70>] (__alloc_pages_nodemask+0x47c/0x4d4) from
>> [<c00a1fd8>] (__get_free_pages+)
>> [ 1712.824645] [<c00a1fd8>] (__get_free_pages+0x10/0x3c) from [<c026eb5c>]
>> (__alloc_skb+0x4c/0xfc)
>> [ 1712.833465] [<c026eb5c>] (__alloc_skb+0x4c/0xfc) from [<bf28c738>]
>> (l2cap_recv_acldata+0xf0/0x1f8 )
>> [ 1712.843322] [<bf28c738>] (l2cap_recv_acldata+0xf0/0x1f8 [l2cap]) from
>> [<bf0094ac>] (hci_rx_task+0x)
>> ...
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> ---
>>  net/bluetooth/l2cap.c |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index ce8f5e4..c95fbd8 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -4654,6 +4654,8 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>>
>>       if (flags & ACL_START) {
>>               struct l2cap_hdr *hdr;
>> +             struct sock *sk;
>> +             u16 cid;
>>               int len;
>>
>>               if (conn->rx_len) {
>> @@ -4673,6 +4675,7 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>>
>>               hdr = (struct l2cap_hdr *) skb->data;
>>               len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
>> +             cid = __le16_to_cpu(hdr->cid);
>>
>>               if (len == skb->len) {
>>                       /* Complete frame received */
>> @@ -4689,6 +4692,20 @@ static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 fl
>>                       goto drop;
>>               }
>>
>> +             sk = l2cap_get_chan_by_scid(&conn->chan_list, cid);
>> +
>> +             if (sk && l2cap_pi(sk)->imtu < len) {
>> +                     BT_ERR("Frame exceeding recv MTU (len %d, MTU %d)",
>> +                                     len, l2cap_pi(sk)->imtu);
>
> I think you have to check if the imtu is less than (len -
> L2CAP_HDR_SIZE) because we don't count the header in the MTU.
>
>> +                     conn->rx_len = 0; /* needed? */
>
> It's not needed. rx_len is always zero here.
>

Thanks, I will fix those issues.

Regards,
Andrei