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 <daniel@xxxxxxxxxxxxx> Cc: Mat Martineau <mathew.j.martineau@xxxxxxxxxxxxxxx> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx> Cc: Willem de Bruijn <willemb@xxxxxxxxxx> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>Acked-by: Mat Martineau <mathew.j.martineau@xxxxxxxxxxxxxxx>--- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
