* [PATCHv3 1/3] Bluetooth: fix MTU L2CAP configuration parameter
2010-09-09 7:42 [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes Emeltchenko Andrei
@ 2010-09-09 7:42 ` Emeltchenko Andrei
2010-09-09 7:43 ` [PATCHv3 2/3] Bluetooth: check for l2cap header in start fragment Emeltchenko Andrei
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Emeltchenko Andrei @ 2010-09-09 7:42 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCHv3 2/3] Bluetooth: check for l2cap header in start fragment
2010-09-09 7:42 [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes Emeltchenko Andrei
2010-09-09 7:42 ` [PATCHv3 1/3] Bluetooth: fix MTU L2CAP configuration parameter Emeltchenko Andrei
@ 2010-09-09 7:43 ` Emeltchenko Andrei
2010-09-09 7:43 ` [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment Emeltchenko Andrei
2010-09-14 13:48 ` [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes Andrei Emeltchenko
3 siblings, 0 replies; 7+ messages in thread
From: Emeltchenko Andrei @ 2010-09-09 7:43 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment
2010-09-09 7:42 [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes Emeltchenko Andrei
2010-09-09 7:42 ` [PATCHv3 1/3] Bluetooth: fix MTU L2CAP configuration parameter Emeltchenko Andrei
2010-09-09 7:43 ` [PATCHv3 2/3] Bluetooth: check for l2cap header in start fragment Emeltchenko Andrei
@ 2010-09-09 7:43 ` Emeltchenko Andrei
2010-09-14 18:21 ` Gustavo F. Padovan
2010-09-14 13:48 ` [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes Andrei Emeltchenko
3 siblings, 1 reply; 7+ messages in thread
From: Emeltchenko Andrei @ 2010-09-09 7:43 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment
2010-09-09 7:43 ` [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment Emeltchenko Andrei
@ 2010-09-14 18:21 ` Gustavo F. Padovan
2010-09-15 11:17 ` Andrei Emeltchenko
0 siblings, 1 reply; 7+ messages in thread
From: Gustavo F. Padovan @ 2010-09-14 18:21 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment
2010-09-14 18:21 ` Gustavo F. Padovan
@ 2010-09-15 11:17 ` Andrei Emeltchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andrei Emeltchenko @ 2010-09-15 11:17 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes
2010-09-09 7:42 [PATCHv3 0/3] Bluetooth: L2CAP robustness fixes Emeltchenko Andrei
` (2 preceding siblings ...)
2010-09-09 7:43 ` [PATCHv3 3/3] Bluetooth: check L2CAP length in first ACL fragment Emeltchenko Andrei
@ 2010-09-14 13:48 ` Andrei Emeltchenko
3 siblings, 0 replies; 7+ messages in thread
From: Andrei Emeltchenko @ 2010-09-14 13:48 UTC (permalink / raw)
To: linux-bluetooth
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread