[PATCH 0/2] Bluetooth: various SCO fixes

[PATCH 0/2] Bluetooth: various SCO fixes

[Desmond Cheong Zhi Xi]

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

[PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del

[Desmond Cheong Zhi Xi]

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

RE: Bluetooth: various SCO fixes

[bluez.test.bot]

[-- 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 --]

Re: [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del

[Marcel Holtmann]

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

Re: [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del

[Desmond Cheong Zhi Xi]

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
> 

[PATCH 2/2] Bluetooth: fix init and cleanup of sco_conn.timeout_work

[Desmond Cheong Zhi Xi]

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

Re: [PATCH 0/2] Bluetooth: various SCO fixes

[Luiz Augusto von Dentz]

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