Return-Path: Message-ID: <578FC1E0.3070803@iogearbox.net> Date: Wed, 20 Jul 2016 20:24:32 +0200 From: Daniel Borkmann MIME-Version: 1.0 To: Mat Martineau CC: johan.hedberg@gmail.com, linux-bluetooth@vger.kernel.org, marcel@holtmann.org, Gustavo Padovan , Willem de Bruijn , Alexei Starovoitov Subject: Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb References: In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed List-ID: On 07/20/2016 06:47 PM, Mat Martineau wrote: > On Wed, 20 Jul 2016, Daniel Borkmann wrote: > >> 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. Since skipping sk_filter() for ERTM >> may have consequences wrt future abi, we need to generally disallow >> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during >> socket initialization. Given that noone run into this issue before as >> it otherwise would have been noticed and fixed, there should be little >> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted >> should there be a use case to call sk_filter() at a safe place for such >> kind of sockets. > > I think the location for a call to sk_filter() for ERTM is early in l2cap_data_rcv(), if that change is needed later on. > >> Fixes: e328140fdacb ("Bluetooth: Use event-driven approach for handling ERTM receive buffer") >> Signed-off-by: Daniel Borkmann >> Cc: Mat Martineau >> Cc: Gustavo Padovan >> Cc: Willem de Bruijn >> Cc: Alexei Starovoitov > > Acked-by: Mat Martineau > >> --- >> I don't have a BT setup at hand, so compile tested only at this time. >> Would be great if some of the BT folks could help out or take over if >> possible. Thanks! >> >> net/bluetooth/l2cap_sock.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c >> index 388ee8b..10ba801 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)) > > It would be more future-proof to check specifically for -ENOMEM and -ENOBUFS, but right now those are the only errors returned by __sock_queue_rcv_skb(). Since there's also core code relying that an error from __sock_queue_rcv_skb() really means we have some recvbuf issue, I think we can spare making this two tests in fast-path. Thanks, Daniel