| Message ID | 20230202090509.2774062-1-iam@sung-woo.kim |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: L2CAP: Fix use-after-free | expand |
On Thu, Feb 2, 2023 at 10:07 AM Sungwoo Kim <iam@sung-woo.kim> wrote: > > Due to the race condition between l2cap_sock_cleanup_listen and > l2cap_sock_close_cb, l2cap_sock_kill can receive already freed sk, > resulting in use-after-free inside l2cap_sock_kill. > This patch prevent this by adding a null check in l2cap_sock_kill. > > Context 1: > l2cap_sock_cleanup_listen(); > // context switched > l2cap_chan_lock(chan); > l2cap_sock_kill(sk); // <-- sk is already freed below But sk is used in l2cap_sock_cleanup_listen() and should not be NULL... while ((sk = bt_accept_dequeue(parent, NULL))) { ... l2cap_sock_kill(sk); .. } It would help if you send us a stack trace ... > > Context 2: > l2cap_chan_timeout(); > l2cap_chan_lock(chan); > chan->ops->close(chan); > l2cap_sock_close_cb() > l2cap_sock_kill(sk); // <-- sk is freed here > l2cap_chan_unlock(chan); > Please add a Fixes: tag > Signed-off-by: Sungwoo Kim <iam@sung-woo.kim> > --- > net/bluetooth/l2cap_sock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index ca8f07f35..657704059 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -1245,7 +1245,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg, > */ > static void l2cap_sock_kill(struct sock *sk) > { > - if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket) > + if (!sk || !sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket) > return; > > BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state)); > -- > 2.25.1 >
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index ca8f07f35..657704059 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -1245,7 +1245,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg, */ static void l2cap_sock_kill(struct sock *sk) { - if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket) + if (!sk || !sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket) return; BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
Due to the race condition between l2cap_sock_cleanup_listen and l2cap_sock_close_cb, l2cap_sock_kill can receive already freed sk, resulting in use-after-free inside l2cap_sock_kill. This patch prevent this by adding a null check in l2cap_sock_kill. Context 1: l2cap_sock_cleanup_listen(); // context switched l2cap_chan_lock(chan); l2cap_sock_kill(sk); // <-- sk is already freed below Context 2: l2cap_chan_timeout(); l2cap_chan_lock(chan); chan->ops->close(chan); l2cap_sock_close_cb() l2cap_sock_kill(sk); // <-- sk is freed here l2cap_chan_unlock(chan); Signed-off-by: Sungwoo Kim <iam@sung-woo.kim> --- net/bluetooth/l2cap_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)