From: Desmond Cheong Zhi Xi <hidden> Date: 2021-09-03 03:23:34
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
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-09-03 03:23:40
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 <redacted>
---
net/bluetooth/sco.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-09-03 03:23:44
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 <redacted>
---
net/bluetooth/sco.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
@@ -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);}
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Date: 2021-09-03 23:37:42
Hi Desmond,
On Thu, Sep 2, 2021 at 8:23 PM Desmond Cheong Zhi Xi
[off-list ref] 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
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 <redacted>
---
net/bluetooth/sco.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-10-04 18:12:13
Hi Marcel,
On 10/9/21 3:36 am, Marcel Holtmann wrote:
Hi Desmond,
quoted
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 <redacted>
---
net/bluetooth/sco.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
/* 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
quoted
+ 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);