LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [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; 6+ 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] 6+ 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-10  7:36   ` 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, 1 reply; 6+ 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	[flat|nested] 6+ 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; 6+ 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	[flat|nested] 6+ 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; 6+ 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] 6+ 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-10  7:36   ` Marcel Holtmann
  2021-10-04 18:12     ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 6+ 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] 6+ messages in thread

* Re: [PATCH 1/2] Bluetooth: call sock_hold earlier in sco_conn_del
  2021-09-10  7:36   ` Marcel Holtmann
@ 2021-10-04 18:12     ` Desmond Cheong Zhi Xi
  0 siblings, 0 replies; 6+ 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] 6+ messages in thread

end of thread, other threads:[~2021-10-04 18:12 UTC | newest]

Thread overview: 6+ 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-10  7:36   ` 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).