* [PATCH 0/2] Bluetooth: various SCO fixes @ 2021-09-03 3:13 Desmond Cheong Zhi Xi 2021-09-03 3:13 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Desmond Cheong Zhi Xi ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-09-03 3:13 UTC (permalink / raw) To: marcel, johan.hedberg, luiz.dentz, davem, kuba Cc: Desmond Cheong Zhi Xi, linux-bluetooth, netdev, linux-kernel, eric.dumazet Hi, This patch set contains some of the fixes for SCO following our discussion on commit ba316be1b6a0 ("Bluetooth: schedule SCO timeouts with delayed_work") [1]. I believe these patches should go in together with [2] to address the UAF errors that have been reported by Syzbot following commit ba316be1b6a0. Link: https://lore.kernel.org/lkml/20210810041410.142035-2-desmondcheongzx@gmail.com/ [1] Link: https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/ [2] Best wishes, Desmond Desmond Cheong Zhi Xi (2): Bluetooth: call sock_hold earlier in sco_conn_del Bluetooth: fix init and cleanup of sco_conn.timeout_work net/bluetooth/sco.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del 2021-09-03 3:13 [PATCH 0/2] Bluetooth: various SCO fixes Desmond Cheong Zhi Xi @ 2021-09-03 3:13 ` Desmond Cheong Zhi Xi 2021-09-03 4:02 ` Bluetooth: various SCO fixes bluez.test.bot 2021-09-10 7:36 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Marcel Holtmann 2021-09-03 3:13 ` [PATCH 2/2] Bluetooth: fix init and cleanup of sco_conn.timeout_work Desmond Cheong Zhi Xi 2021-09-03 23:37 ` [PATCH 0/2] Bluetooth: various SCO fixes Luiz Augusto von Dentz 2 siblings, 2 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-09-03 3:13 UTC (permalink / raw) To: marcel, johan.hedberg, luiz.dentz, davem, kuba Cc: Desmond Cheong Zhi Xi, linux-bluetooth, netdev, linux-kernel, eric.dumazet In sco_conn_del, conn->sk is read while holding on to the sco_conn.lock to avoid races with a socket that could be released concurrently. However, in between unlocking sco_conn.lock and calling sock_hold, it's possible for the socket to be freed, which would cause a use-after-free write when sock_hold is finally called. To fix this, the reference count of the socket should be increased while the sco_conn.lock is still held. Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- net/bluetooth/sco.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index b62c91c627e2..4a057f99b60a 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err) /* Kill socket */ sco_conn_lock(conn); sk = conn->sk; + if (sk) + sock_hold(sk); sco_conn_unlock(conn); if (sk) { - sock_hold(sk); lock_sock(sk); sco_sock_clear_timer(sk); sco_chan_del(sk, err); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: Bluetooth: various SCO fixes 2021-09-03 3:13 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Desmond Cheong Zhi Xi @ 2021-09-03 4:02 ` bluez.test.bot 2021-09-10 7:36 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Marcel Holtmann 1 sibling, 0 replies; 7+ messages in thread From: bluez.test.bot @ 2021-09-03 4:02 UTC (permalink / raw) To: linux-bluetooth, desmondcheongzx [-- Attachment #1: Type: text/plain, Size: 3269 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=541523 ---Test result--- Test Summary: CheckPatch FAIL 0.77 seconds GitLint PASS 0.21 seconds BuildKernel PASS 519.76 seconds TestRunner: Setup PASS 348.61 seconds TestRunner: l2cap-tester PASS 2.54 seconds TestRunner: bnep-tester PASS 1.87 seconds TestRunner: mgmt-tester PASS 30.24 seconds TestRunner: rfcomm-tester PASS 2.08 seconds TestRunner: sco-tester PASS 1.98 seconds TestRunner: smp-tester PASS 2.08 seconds TestRunner: userchan-tester PASS 1.89 seconds Details ############################## Test: CheckPatch - FAIL - 0.77 seconds Run checkpatch.pl script with rule in .checkpatch.conf Bluetooth: fix init and cleanup of sco_conn.timeout_work WARNING: Unknown commit id 'ba316be1b6a0', maybe rebased or not pulled? #16: Fixes: ba316be1b6a0 ("Bluetooth: schedule SCO timeouts with delayed_work") total: 0 errors, 1 warnings, 0 checks, 29 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. "[PATCH] Bluetooth: fix init and cleanup of sco_conn.timeout_work" has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. ############################## Test: GitLint - PASS - 0.21 seconds Run gitlint with rule in .gitlint ############################## Test: BuildKernel - PASS - 519.76 seconds Build Kernel with minimal configuration supports Bluetooth ############################## Test: TestRunner: Setup - PASS - 348.61 seconds Setup environment for running Test Runner ############################## Test: TestRunner: l2cap-tester - PASS - 2.54 seconds Run test-runner with l2cap-tester Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: bnep-tester - PASS - 1.87 seconds Run test-runner with bnep-tester Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: mgmt-tester - PASS - 30.24 seconds Run test-runner with mgmt-tester Total: 452, Passed: 452 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: rfcomm-tester - PASS - 2.08 seconds Run test-runner with rfcomm-tester Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: sco-tester - PASS - 1.98 seconds Run test-runner with sco-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: smp-tester - PASS - 2.08 seconds Run test-runner with smp-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: userchan-tester - PASS - 1.89 seconds Run test-runner with userchan-tester Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth [-- Attachment #2: l2cap-tester.log --] [-- Type: application/octet-stream, Size: 44385 bytes --] [-- Attachment #3: bnep-tester.log --] [-- Type: application/octet-stream, Size: 3592 bytes --] [-- Attachment #4: mgmt-tester.log --] [-- Type: application/octet-stream, Size: 622560 bytes --] [-- Attachment #5: rfcomm-tester.log --] [-- Type: application/octet-stream, Size: 11712 bytes --] [-- Attachment #6: sco-tester.log --] [-- Type: application/octet-stream, Size: 9947 bytes --] [-- Attachment #7: smp-tester.log --] [-- Type: application/octet-stream, Size: 11857 bytes --] [-- Attachment #8: userchan-tester.log --] [-- Type: application/octet-stream, Size: 5489 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del 2021-09-03 3:13 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Desmond Cheong Zhi Xi 2021-09-03 4:02 ` Bluetooth: various SCO fixes bluez.test.bot @ 2021-09-10 7:36 ` Marcel Holtmann 2021-10-04 18:12 ` Desmond Cheong Zhi Xi 1 sibling, 1 reply; 7+ messages in thread From: Marcel Holtmann @ 2021-09-10 7:36 UTC (permalink / raw) To: Desmond Cheong Zhi Xi Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller, Jakub Kicinski, linux-bluetooth, open list:NETWORKING [GENERAL], open list, eric.dumazet Hi Desmond, > In sco_conn_del, conn->sk is read while holding on to the > sco_conn.lock to avoid races with a socket that could be released > concurrently. > > However, in between unlocking sco_conn.lock and calling sock_hold, > it's possible for the socket to be freed, which would cause a > use-after-free write when sock_hold is finally called. > > To fix this, the reference count of the socket should be increased > while the sco_conn.lock is still held. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > --- > net/bluetooth/sco.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index b62c91c627e2..4a057f99b60a 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err) > /* Kill socket */ > sco_conn_lock(conn); > sk = conn->sk; please add a comment here on why we are doing it. > + if (sk) > + sock_hold(sk); > sco_conn_unlock(conn); > > if (sk) { > - sock_hold(sk); > lock_sock(sk); > sco_sock_clear_timer(sk); > sco_chan_del(sk, err); Regards Marcel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del 2021-09-10 7:36 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Marcel Holtmann @ 2021-10-04 18:12 ` Desmond Cheong Zhi Xi 0 siblings, 0 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-10-04 18:12 UTC (permalink / raw) To: Marcel Holtmann Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller, Jakub Kicinski, linux-bluetooth, open list:NETWORKING [GENERAL], open list, eric.dumazet Hi Marcel, On 10/9/21 3:36 am, Marcel Holtmann wrote: > Hi Desmond, > >> In sco_conn_del, conn->sk is read while holding on to the >> sco_conn.lock to avoid races with a socket that could be released >> concurrently. >> >> However, in between unlocking sco_conn.lock and calling sock_hold, >> it's possible for the socket to be freed, which would cause a >> use-after-free write when sock_hold is finally called. >> >> To fix this, the reference count of the socket should be increased >> while the sco_conn.lock is still held. >> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> --- >> net/bluetooth/sco.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c >> index b62c91c627e2..4a057f99b60a 100644 >> --- a/net/bluetooth/sco.c >> +++ b/net/bluetooth/sco.c >> @@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err) >> /* Kill socket */ >> sco_conn_lock(conn); >> sk = conn->sk; > > please add a comment here on why we are doing it. > So sorry for the very delayed response. I was looking through old email threads to check if my recently resent patch was still necessary, and just realized I missed this email. This patch was merged into the bluetooth-next tree before your feedback came in. Would you still like me to write a separate patch to add the requested comment? Best wishes, Desmond >> + if (sk) >> + sock_hold(sk); >> sco_conn_unlock(conn); >> >> if (sk) { >> - sock_hold(sk); >> lock_sock(sk); >> sco_sock_clear_timer(sk); >> sco_chan_del(sk, err); > > Regards > > Marcel > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] Bluetooth: fix init and cleanup of sco_conn.timeout_work 2021-09-03 3:13 [PATCH 0/2] Bluetooth: various SCO fixes Desmond Cheong Zhi Xi 2021-09-03 3:13 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Desmond Cheong Zhi Xi @ 2021-09-03 3:13 ` Desmond Cheong Zhi Xi 2021-09-03 23:37 ` [PATCH 0/2] Bluetooth: various SCO fixes Luiz Augusto von Dentz 2 siblings, 0 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-09-03 3:13 UTC (permalink / raw) To: marcel, johan.hedberg, luiz.dentz, davem, kuba Cc: Desmond Cheong Zhi Xi, linux-bluetooth, netdev, linux-kernel, eric.dumazet Before freeing struct sco_conn, all delayed timeout work should be cancelled. Otherwise, sco_sock_timeout could potentially use the sco_conn after it has been freed. Additionally, sco_conn.timeout_work should be initialized when the connection is allocated, not when the channel is added. This is because an sco_conn can create channels with multiple sockets over its lifetime, which happens if sockets are released but the connection isn't deleted. Fixes: ba316be1b6a0 ("Bluetooth: schedule SCO timeouts with delayed_work") Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- net/bluetooth/sco.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 4a057f99b60a..6e047e178c0a 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -133,6 +133,7 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon) return NULL; spin_lock_init(&conn->lock); + INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout); hcon->sco_data = conn; conn->hcon = hcon; @@ -197,11 +198,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err) sco_chan_del(sk, err); release_sock(sk); sock_put(sk); - - /* Ensure no more work items will run before freeing conn. */ - cancel_delayed_work_sync(&conn->timeout_work); } + /* Ensure no more work items will run before freeing conn. */ + cancel_delayed_work_sync(&conn->timeout_work); + hcon->sco_data = NULL; kfree(conn); } @@ -214,8 +215,6 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk, sco_pi(sk)->conn = conn; conn->sk = sk; - INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout); - if (parent) bt_accept_enqueue(parent, sk, true); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Bluetooth: various SCO fixes 2021-09-03 3:13 [PATCH 0/2] Bluetooth: various SCO fixes Desmond Cheong Zhi Xi 2021-09-03 3:13 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Desmond Cheong Zhi Xi 2021-09-03 3:13 ` [PATCH 2/2] Bluetooth: fix init and cleanup of sco_conn.timeout_work Desmond Cheong Zhi Xi @ 2021-09-03 23:37 ` Luiz Augusto von Dentz 2 siblings, 0 replies; 7+ messages in thread From: Luiz Augusto von Dentz @ 2021-09-03 23:37 UTC (permalink / raw) To: Desmond Cheong Zhi Xi Cc: Marcel Holtmann, Johan Hedberg, David Miller, Jakub Kicinski, linux-bluetooth, open list:NETWORKING [GENERAL], Linux Kernel Mailing List, Eric Dumazet Hi Desmond, On Thu, Sep 2, 2021 at 8:23 PM Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote: > > > Hi, > > This patch set contains some of the fixes for SCO following our > discussion on commit ba316be1b6a0 ("Bluetooth: schedule SCO timeouts > with delayed_work") [1]. > > I believe these patches should go in together with [2] to address the > UAF errors that have been reported by Syzbot following > commit ba316be1b6a0. > > Link: https://lore.kernel.org/lkml/20210810041410.142035-2-desmondcheongzx@gmail.com/ [1] > Link: https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/ [2] > > Best wishes, > Desmond > > Desmond Cheong Zhi Xi (2): > Bluetooth: call sock_hold earlier in sco_conn_del > Bluetooth: fix init and cleanup of sco_conn.timeout_work > > net/bluetooth/sco.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > -- > 2.25.1 Applied, thanks. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-04 18:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-03 3:13 [PATCH 0/2] Bluetooth: various SCO fixes Desmond Cheong Zhi Xi 2021-09-03 3:13 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Desmond Cheong Zhi Xi 2021-09-03 4:02 ` Bluetooth: various SCO fixes bluez.test.bot 2021-09-10 7:36 ` [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del Marcel Holtmann 2021-10-04 18:12 ` Desmond Cheong Zhi Xi 2021-09-03 3:13 ` [PATCH 2/2] Bluetooth: fix init and cleanup of sco_conn.timeout_work Desmond Cheong Zhi Xi 2021-09-03 23:37 ` [PATCH 0/2] Bluetooth: various SCO fixes Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).