From: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> To: marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com, davem@davemloft.net, kuba@kernel.org, sudipm.mukherjee@gmail.com Cc: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>, linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, skhan@linuxfoundation.org, gregkh@linuxfoundation.org, linux-kernel-mentees@lists.linuxfoundation.org Subject: [PATCH v6 6/6] Bluetooth: fix repeated calls to sco_sock_kill Date: Tue, 10 Aug 2021 12:14:10 +0800 [thread overview] Message-ID: <20210810041410.142035-7-desmondcheongzx@gmail.com> (raw) In-Reply-To: <20210810041410.142035-1-desmondcheongzx@gmail.com> In commit 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket"), a check was added to sco_sock_kill to skip killing a socket if the SOCK_DEAD flag was set. This was done after a trace for a use-after-free bug showed that the same sock pointer was being killed twice. Unfortunately, this check prevents sco_sock_kill from running on any socket. sco_sock_kill kills a socket only if it's zapped and orphaned, however sock_orphan announces that the socket is dead before detaching it. i.e., orphaned sockets have the SOCK_DEAD flag set. To fix this, we remove the check for SOCK_DEAD, and avoid repeated calls to sco_sock_kill by removing incorrect calls in: 1. sco_sock_timeout. The socket should not be killed on timeout as further processing is expected to be done. For example, sco_sock_connect sets the timer then waits for the socket to be connected or for an error to be returned. 2. sco_conn_del. This function should clean up resources for the connection, but the socket itself should be cleaned up in sco_sock_release. 3. sco_sock_close. Calls to sco_sock_close in sco_sock_cleanup_listen and sco_sock_release are followed by sco_sock_kill. Hence the duplicated call should be removed. Fixes: 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket") Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- net/bluetooth/sco.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 77490338f4fa..98a881586512 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -97,8 +97,6 @@ static void sco_sock_timeout(struct work_struct *work) sk->sk_err = ETIMEDOUT; sk->sk_state_change(sk); release_sock(sk); - - sco_sock_kill(sk); sock_put(sk); } @@ -197,7 +195,6 @@ static void sco_conn_del(struct hci_conn *hcon, int err) sco_sock_clear_timer(sk); sco_chan_del(sk, err); release_sock(sk); - sco_sock_kill(sk); sock_put(sk); /* Ensure no more work items will run before freeing conn. */ @@ -404,8 +401,7 @@ static void sco_sock_cleanup_listen(struct sock *parent) */ static void sco_sock_kill(struct sock *sk) { - if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket || - sock_flag(sk, SOCK_DEAD)) + if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket) return; BT_DBG("sk %p state %d", sk, sk->sk_state); @@ -457,7 +453,6 @@ static void sco_sock_close(struct sock *sk) sco_sock_clear_timer(sk); __sco_sock_close(sk); release_sock(sk); - sco_sock_kill(sk); } static void sco_skb_put_cmsg(struct sk_buff *skb, struct msghdr *msg, -- 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> To: marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com, davem@davemloft.net, kuba@kernel.org, sudipm.mukherjee@gmail.com Cc: linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>, linux-kernel-mentees@lists.linuxfoundation.org Subject: [PATCH v6 6/6] Bluetooth: fix repeated calls to sco_sock_kill Date: Tue, 10 Aug 2021 12:14:10 +0800 [thread overview] Message-ID: <20210810041410.142035-7-desmondcheongzx@gmail.com> (raw) In-Reply-To: <20210810041410.142035-1-desmondcheongzx@gmail.com> In commit 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket"), a check was added to sco_sock_kill to skip killing a socket if the SOCK_DEAD flag was set. This was done after a trace for a use-after-free bug showed that the same sock pointer was being killed twice. Unfortunately, this check prevents sco_sock_kill from running on any socket. sco_sock_kill kills a socket only if it's zapped and orphaned, however sock_orphan announces that the socket is dead before detaching it. i.e., orphaned sockets have the SOCK_DEAD flag set. To fix this, we remove the check for SOCK_DEAD, and avoid repeated calls to sco_sock_kill by removing incorrect calls in: 1. sco_sock_timeout. The socket should not be killed on timeout as further processing is expected to be done. For example, sco_sock_connect sets the timer then waits for the socket to be connected or for an error to be returned. 2. sco_conn_del. This function should clean up resources for the connection, but the socket itself should be cleaned up in sco_sock_release. 3. sco_sock_close. Calls to sco_sock_close in sco_sock_cleanup_listen and sco_sock_release are followed by sco_sock_kill. Hence the duplicated call should be removed. Fixes: 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket") Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- net/bluetooth/sco.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 77490338f4fa..98a881586512 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -97,8 +97,6 @@ static void sco_sock_timeout(struct work_struct *work) sk->sk_err = ETIMEDOUT; sk->sk_state_change(sk); release_sock(sk); - - sco_sock_kill(sk); sock_put(sk); } @@ -197,7 +195,6 @@ static void sco_conn_del(struct hci_conn *hcon, int err) sco_sock_clear_timer(sk); sco_chan_del(sk, err); release_sock(sk); - sco_sock_kill(sk); sock_put(sk); /* Ensure no more work items will run before freeing conn. */ @@ -404,8 +401,7 @@ static void sco_sock_cleanup_listen(struct sock *parent) */ static void sco_sock_kill(struct sock *sk) { - if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket || - sock_flag(sk, SOCK_DEAD)) + if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket) return; BT_DBG("sk %p state %d", sk, sk->sk_state); @@ -457,7 +453,6 @@ static void sco_sock_close(struct sock *sk) sco_sock_clear_timer(sk); __sco_sock_close(sk); release_sock(sk); - sco_sock_kill(sk); } static void sco_skb_put_cmsg(struct sk_buff *skb, struct msghdr *msg, -- 2.25.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2021-08-10 4:18 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-10 4:14 [PATCH v6 0/6] Bluetooth: fix locking and socket killing in SCO and RFCOMM Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 5:14 ` Bluetooth: fix locking and socket killing in SCO and RFCOMM bluez.test.bot 2021-08-10 17:51 ` Luiz Augusto von Dentz 2021-09-02 19:17 ` [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work Eric Dumazet 2021-09-02 19:17 ` Eric Dumazet 2021-09-02 19:32 ` Desmond Cheong Zhi Xi 2021-09-02 19:32 ` Desmond Cheong Zhi Xi 2021-09-02 21:41 ` Eric Dumazet 2021-09-02 21:41 ` Eric Dumazet 2021-09-02 22:53 ` Desmond Cheong Zhi Xi 2021-09-02 22:53 ` Desmond Cheong Zhi Xi 2021-09-02 23:05 ` Desmond Cheong Zhi Xi 2021-09-02 23:05 ` Desmond Cheong Zhi Xi 2021-09-02 23:42 ` Luiz Augusto von Dentz 2021-09-02 23:42 ` Luiz Augusto von Dentz 2021-09-03 3:17 ` Desmond Cheong Zhi Xi 2021-09-03 3:17 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 2/6] Bluetooth: avoid circular locks in sco_sock_connect Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 3/6] Bluetooth: switch to lock_sock in SCO Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 4/6] Bluetooth: serialize calls to sco_sock_{set,clear}_timer Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 4/6] Bluetooth: serialize calls to sco_sock_{set, clear}_timer Desmond Cheong Zhi Xi 2021-08-10 4:14 ` [PATCH v6 5/6] Bluetooth: switch to lock_sock in RFCOMM Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi 2021-08-10 4:14 ` Desmond Cheong Zhi Xi [this message] 2021-08-10 4:14 ` [PATCH v6 6/6] Bluetooth: fix repeated calls to sco_sock_kill Desmond Cheong Zhi Xi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210810041410.142035-7-desmondcheongzx@gmail.com \ --to=desmondcheongzx@gmail.com \ --cc=davem@davemloft.net \ --cc=gregkh@linuxfoundation.org \ --cc=johan.hedberg@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-bluetooth@vger.kernel.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luiz.dentz@gmail.com \ --cc=marcel@holtmann.org \ --cc=netdev@vger.kernel.org \ --cc=skhan@linuxfoundation.org \ --cc=sudipm.mukherjee@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.