[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

[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

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
>