[PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Mat Martineau]

From: Daniel Borkmann <daniel@iogearbox.net>

During an audit for sk_filter(), we found that rx_busy_skb handling
in l2cap_sock_recv_cb() and l2cap_sock_recvmsg() looks not quite as
intended.

The assumption from commit e328140fdacb ("Bluetooth: Use event-driven
approach for handling ERTM receive buffer") is that errors returned
from sock_queue_rcv_skb() are due to receive buffer shortage. However,
nothing should prevent doing a setsockopt() with SO_ATTACH_FILTER on
the socket, that could drop some of the incoming skbs when handled in
sock_queue_rcv_skb().

In that case sock_queue_rcv_skb() will return with -EPERM, propagated
from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
that we failed due to receive buffer being full. From that point onwards,
due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
any forward progress as rx_busy_skb is never cleared from l2cap_sock_recvmsg(),
due to the filter drop verdict over and over coming from sk_filter().
Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
dropped due to rx_busy_skb being occupied.

Instead, just use __sock_queue_rcv_skb() where an error really tells
that there's a receive buffer issue. Split the sk_filter() and only
enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
skb has already been through the ERTM state machine and it has been
acked, so dropping is not allowed. Instead, for ERTM, call sk_filter in
l2cap_data_rcv() so the packet can be dropped before the state machine
sees it.

Fixes: e328140fdacb ("Bluetooth: Use event-driven approach for handling ERTM receive buffer")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Alexei Starovoitov <ast@kernel.org>
---

I modified the original patch to call sk_filter for ERTM before the packet is
handled by the state machine and to not set the filter locked flag. I tested
using l2test in ERTM mode, with and without a "randomly drop 1 in 64 packets"
filter attached.

Mat

---
 net/bluetooth/l2cap_core.c |  4 ++++
 net/bluetooth/l2cap_sock.c | 13 +++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 54ceb1f..d5de0ce 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -32,6 +32,7 @@
 
 #include <linux/debugfs.h>
 #include <linux/crc16.h>
+#include <linux/filter.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -6610,6 +6611,9 @@ static int l2cap_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
 		goto drop;
 	}
 
+	if (chan->mode == L2CAP_MODE_ERTM && sk_filter(chan->data, skb))
+		goto drop;
+
 	if (!control->sframe) {
 		int err;
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1842141..94daa2e 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1019,7 +1019,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		goto done;
 
 	if (pi->rx_busy_skb) {
-		if (!sock_queue_rcv_skb(sk, pi->rx_busy_skb))
+		if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
 			pi->rx_busy_skb = NULL;
 		else
 			goto done;
@@ -1270,7 +1270,16 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 		goto done;
 	}
 
-	err = sock_queue_rcv_skb(sk, skb);
+	if (chan->mode != L2CAP_MODE_ERTM) {
+		/* Even if no filter is attached, we could potentially
+		 * get errors from security modules, etc.
+		 */
+		err = sk_filter(sk, skb);
+		if (err)
+			goto done;
+	}
+
+	err = __sock_queue_rcv_skb(sk, skb);
 
 	/* For ERTM, handle one skb that doesn't fit into the recv
 	 * buffer.  This is important to do because the data frames
-- 
2.9.2

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Willem de Bruijn]

> I modified the original patch to call sk_filter for ERTM before the packet is
> handled by the state machine and to not set the filter locked flag. I tested
> using l2test in ERTM mode, with and without a "randomly drop 1 in 64 packets"
> filter attached.

Thanks for testing. For consistency's sake, is it preferable to filter
at this point for all modes?

>
> Mat
>
> ---
>  net/bluetooth/l2cap_core.c |  4 ++++
>  net/bluetooth/l2cap_sock.c | 13 +++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 54ceb1f..d5de0ce 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -32,6 +32,7 @@
>
>  #include <linux/debugfs.h>
>  #include <linux/crc16.h>
> +#include <linux/filter.h>
>
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -6610,6 +6611,9 @@ static int l2cap_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
>                 goto drop;
>         }
>
> +       if (chan->mode == L2CAP_MODE_ERTM && sk_filter(chan->data, skb))
> +               goto drop;
> +

sk_filter can also accept, but trim, packets. If the protocol expects
a header that it unconditionally pulls later, use sk_filter_trim to
avoid trimming to below header length. I think I saw a path from

  l2cap_data_rcv
    l2cap_rx
      l2cap_rx_state_recv
        l2cap_reassemble_sdu
          case L2CAP_SAR_START
            skb_pull(skb, L2CAP_SDULEN_SIZE)

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Mat Martineau]

On Tue, 26 Jul 2016, Willem de Bruijn wrote:

>> I modified the original patch to call sk_filter for ERTM before the packet is
>> handled by the state machine and to not set the filter locked flag. I tested
>> using l2test in ERTM mode, with and without a "randomly drop 1 in 64 packets"
>> filter attached.
>
> Thanks for testing. For consistency's sake, is it preferable to filter
> at this point for all modes?

Only ERTM and streaming mode end up on this code path, and I think there's 
a benefit to handling these two modes similarly. There are a number of 
other paths to l2cap_sock_recv_cb(), and there isn't one perfect place to 
call sk_filter for all modes.

>
>>
>> Mat
>>
>> ---
>>  net/bluetooth/l2cap_core.c |  4 ++++
>>  net/bluetooth/l2cap_sock.c | 13 +++++++++++--
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 54ceb1f..d5de0ce 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -32,6 +32,7 @@
>>
>>  #include <linux/debugfs.h>
>>  #include <linux/crc16.h>
>> +#include <linux/filter.h>
>>
>>  #include <net/bluetooth/bluetooth.h>
>>  #include <net/bluetooth/hci_core.h>
>> @@ -6610,6 +6611,9 @@ static int l2cap_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
>>                 goto drop;
>>         }
>>
>> +       if (chan->mode == L2CAP_MODE_ERTM && sk_filter(chan->data, skb))
>> +               goto drop;
>> +
>
> sk_filter can also accept, but trim, packets. If the protocol expects
> a header that it unconditionally pulls later, use sk_filter_trim to

sk_filter_trim_cap? I see that you added that recently. It's not in 
bluetooth-next and we're aiming for a patch that can be easily backported 
to stable.

> avoid trimming to below header length. I think I saw a path from
>
>  l2cap_data_rcv
>    l2cap_rx
>      l2cap_rx_state_recv
>        l2cap_reassemble_sdu
>          case L2CAP_SAR_START
>            skb_pull(skb, L2CAP_SDULEN_SIZE)

How about checking skb->len before that skb_pull instead?

--
Mat Martineau
Intel OTC

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Willem de Bruijn]

On Tue, Jul 26, 2016 at 1:00 PM, Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Tue, 26 Jul 2016, Willem de Bruijn wrote:
>
>>> I modified the original patch to call sk_filter for ERTM before the
>>> packet is
>>> handled by the state machine and to not set the filter locked flag. I
>>> tested
>>> using l2test in ERTM mode, with and without a "randomly drop 1 in 64
>>> packets"
>>> filter attached.
>>
>>
>> Thanks for testing. For consistency's sake, is it preferable to filter
>> at this point for all modes?
>
>
> Only ERTM and streaming mode end up on this code path, and I think there's a
> benefit to handling these two modes similarly. There are a number of other
> paths to l2cap_sock_recv_cb(), and there isn't one perfect place to call
> sk_filter for all modes.

Okay.
>
>>
>>>
>>> Mat
>>>
>>> ---
>>>  net/bluetooth/l2cap_core.c |  4 ++++
>>>  net/bluetooth/l2cap_sock.c | 13 +++++++++++--
>>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index 54ceb1f..d5de0ce 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -32,6 +32,7 @@
>>>
>>>  #include <linux/debugfs.h>
>>>  #include <linux/crc16.h>
>>> +#include <linux/filter.h>
>>>
>>>  #include <net/bluetooth/bluetooth.h>
>>>  #include <net/bluetooth/hci_core.h>
>>> @@ -6610,6 +6611,9 @@ static int l2cap_data_rcv(struct l2cap_chan *chan,
>>> struct sk_buff *skb)
>>>                 goto drop;
>>>         }
>>>
>>> +       if (chan->mode == L2CAP_MODE_ERTM && sk_filter(chan->data, skb))
>>> +               goto drop;
>>> +
>>
>>
>> sk_filter can also accept, but trim, packets. If the protocol expects
>> a header that it unconditionally pulls later, use sk_filter_trim to
>
>
> sk_filter_trim_cap? I see that you added that recently. It's not in
> bluetooth-next and we're aiming for a patch that can be easily backported to
> stable.
>
>> avoid trimming to below header length. I think I saw a path from
>>
>>  l2cap_data_rcv
>>    l2cap_rx
>>      l2cap_rx_state_recv
>>        l2cap_reassemble_sdu
>>          case L2CAP_SAR_START
>>            skb_pull(skb, L2CAP_SDULEN_SIZE)
>
>
> How about checking skb->len before that skb_pull instead?

That works, preferably using pskb_may_pull. The only downside of doing
that exactly in this path, is that it is not easy to verify that all
other code paths downstream from sk_filter are also safe. If all
expect this header, the check is best added right after filter.

>
> --
> Mat Martineau
> Intel OTC

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Mat Martineau]

On Tue, 26 Jul 2016, Willem de Bruijn wrote:

> On Tue, Jul 26, 2016 at 1:00 PM, Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Tue, 26 Jul 2016, Willem de Bruijn wrote:
>>
>>> sk_filter can also accept, but trim, packets. If the protocol expects
>>> a header that it unconditionally pulls later, use sk_filter_trim to
>>
>>
>> sk_filter_trim_cap? I see that you added that recently. It's not in
>> bluetooth-next and we're aiming for a patch that can be easily backported to
>> stable.
>>
>>> avoid trimming to below header length. I think I saw a path from
>>>
>>>  l2cap_data_rcv
>>>    l2cap_rx
>>>      l2cap_rx_state_recv
>>>        l2cap_reassemble_sdu
>>>          case L2CAP_SAR_START
>>>            skb_pull(skb, L2CAP_SDULEN_SIZE)
>>
>>
>> How about checking skb->len before that skb_pull instead?
>
> That works, preferably using pskb_may_pull. The only downside of doing
> that exactly in this path, is that it is not easy to verify that all
> other code paths downstream from sk_filter are also safe. If all
> expect this header, the check is best added right after filter.

The SDULEN header is only present for SAR_START frames (handled on this 
code path), and there is no other manipulation of skb data/tail/len after 
sk_filter. We know these skbs are linear, or at least they were before 
sk_filter was called. Can sk_filter fragment an otherwise linear skb? 
(Looks like the answer is "no" but I'd like to confirm)

--
Mat Martineau
Intel OTC

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Willem de Bruijn]

On Tue, Jul 26, 2016 at 2:48 PM, Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Tue, 26 Jul 2016, Willem de Bruijn wrote:
>
>> On Tue, Jul 26, 2016 at 1:00 PM, Mat Martineau
>> <mathew.j.martineau@linux.intel.com> wrote:
>>>
>>>
>>> On Tue, 26 Jul 2016, Willem de Bruijn wrote:
>>>
>>>> sk_filter can also accept, but trim, packets. If the protocol expects
>>>> a header that it unconditionally pulls later, use sk_filter_trim to
>>>
>>>
>>>
>>> sk_filter_trim_cap? I see that you added that recently. It's not in
>>> bluetooth-next and we're aiming for a patch that can be easily backported
>>> to
>>> stable.
>>>
>>>> avoid trimming to below header length. I think I saw a path from
>>>>
>>>>  l2cap_data_rcv
>>>>    l2cap_rx
>>>>      l2cap_rx_state_recv
>>>>        l2cap_reassemble_sdu
>>>>          case L2CAP_SAR_START
>>>>            skb_pull(skb, L2CAP_SDULEN_SIZE)
>>>
>>>
>>>
>>> How about checking skb->len before that skb_pull instead?
>>
>>
>> That works, preferably using pskb_may_pull. The only downside of doing
>> that exactly in this path, is that it is not easy to verify that all
>> other code paths downstream from sk_filter are also safe. If all
>> expect this header, the check is best added right after filter.
>
>
> The SDULEN header is only present for SAR_START frames (handled on this code
> path), and there is no other manipulation of skb data/tail/len after
> sk_filter.

Then this is great.

> We know these skbs are linear, or at least they were before
> sk_filter was called. Can sk_filter fragment an otherwise linear skb? (Looks
> like the answer is "no" but I'd like to confirm)

No. It can call ___pskb_trim, but that only happens for non-linear skbs.

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Marcel Holtmann]

Hi Mat,

>>> I modified the original patch to call sk_filter for ERTM before the packet is
>>> handled by the state machine and to not set the filter locked flag. I tested
>>> using l2test in ERTM mode, with and without a "randomly drop 1 in 64 packets"
>>> filter attached.
>> 
>> Thanks for testing. For consistency's sake, is it preferable to filter
>> at this point for all modes?
> 
> Only ERTM and streaming mode end up on this code path, and I think there's a benefit to handling these two modes similarly. There are a number of other paths to l2cap_sock_recv_cb(), and there isn't one perfect place to call sk_filter for all modes.

would code restructuring help to create a better place to put sk_filter?

>>> 
>>> ---
>>> net/bluetooth/l2cap_core.c |  4 ++++
>>> net/bluetooth/l2cap_sock.c | 13 +++++++++++--
>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index 54ceb1f..d5de0ce 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -32,6 +32,7 @@
>>> 
>>> #include <linux/debugfs.h>
>>> #include <linux/crc16.h>
>>> +#include <linux/filter.h>
>>> 
>>> #include <net/bluetooth/bluetooth.h>
>>> #include <net/bluetooth/hci_core.h>
>>> @@ -6610,6 +6611,9 @@ static int l2cap_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
>>>                goto drop;
>>>        }
>>> 
>>> +       if (chan->mode == L2CAP_MODE_ERTM && sk_filter(chan->data, skb))
>>> +               goto drop;
>>> +
>> 
>> sk_filter can also accept, but trim, packets. If the protocol expects
>> a header that it unconditionally pulls later, use sk_filter_trim to
> 
> sk_filter_trim_cap? I see that you added that recently. It's not in bluetooth-next and we're aiming for a patch that can be easily backported to stable.

Lets create a version that fixes this first. One version that we can backport into stable. And then we can start utilizing newer infrastructure available in Linus' tree.

Regards

Marcel

Re: [PATCH v2] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Mat Martineau]

On Wed, 27 Jul 2016, Marcel Holtmann wrote:

> Hi Mat,
>
>>>> I modified the original patch to call sk_filter for ERTM before the packet is
>>>> handled by the state machine and to not set the filter locked flag. I tested
>>>> using l2test in ERTM mode, with and without a "randomly drop 1 in 64 packets"
>>>> filter attached.
>>>
>>> Thanks for testing. For consistency's sake, is it preferable to filter
>>> at this point for all modes?
>>
>> Only ERTM and streaming mode end up on this code path, and I think there's a benefit to handling these two modes similarly. There are a number of other paths to l2cap_sock_recv_cb(), and there isn't one perfect place to call sk_filter for all modes.
>
> would code restructuring help to create a better place to put sk_filter?

Maybe, but I'm not sure it's worthwhile for this purpose alone. The 
handling for ERTM is similar to what I see for TCP, but for LE we want to 
issue credits and do some more reassembly before filtering.

>
>>>>
>>>> ---
>>>> net/bluetooth/l2cap_core.c |  4 ++++
>>>> net/bluetooth/l2cap_sock.c | 13 +++++++++++--
>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>>> index 54ceb1f..d5de0ce 100644
>>>> --- a/net/bluetooth/l2cap_core.c
>>>> +++ b/net/bluetooth/l2cap_core.c
>>>> @@ -32,6 +32,7 @@
>>>>
>>>> #include <linux/debugfs.h>
>>>> #include <linux/crc16.h>
>>>> +#include <linux/filter.h>
>>>>
>>>> #include <net/bluetooth/bluetooth.h>
>>>> #include <net/bluetooth/hci_core.h>
>>>> @@ -6610,6 +6611,9 @@ static int l2cap_data_rcv(struct l2cap_chan *chan, struct sk_buff *skb)
>>>>                goto drop;
>>>>        }
>>>>
>>>> +       if (chan->mode == L2CAP_MODE_ERTM && sk_filter(chan->data, skb))
>>>> +               goto drop;
>>>> +
>>>
>>> sk_filter can also accept, but trim, packets. If the protocol expects
>>> a header that it unconditionally pulls later, use sk_filter_trim to
>>
>> sk_filter_trim_cap? I see that you added that recently. It's not in bluetooth-next and we're aiming for a patch that can be easily backported to stable.
>
> Lets create a version that fixes this first. One version that we can backport into stable. And then we can start utilizing newer infrastructure available in Linus' tree.

I believe v3 is a fix suitable for stable.

--
Mat Martineau
Intel OTC