From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-04 15:48:33
Apologies, resending because I botched the previous cover-letter and
linux-bluetooth was left out of the cc. Just noticed when the
bluez.test.bot didn't respond. Please reply to this series rather than
the one sent previously.
Hi,
This patch series started out as a fix for "inconsistent lock state in
sco_sock_timeout" reported by Syzbot [1].
Patch 1 is sufficient to fix this error. This was also confirmed by the
reproducer for "BUG: corrupted list in kobject_add_internal (3)" [2]
which consistently hits the inconsistent lock state error.
However, while testing the proposed fix, the reproducer for [1] would
randomly return a human-unreadable error [3]. After further
investigation, this bug seems to be caused by an unrelated error with
forking [4].
While trying to fix the mysterious error, additional fixes were added,
such as switching to lock_sock and serializing _{set,clear}_timer.
Additionally, as the reproducer kept hitting the oom-killer, a fix for
SCO socket killing was also added.
The reproducer for [1] was robust enough to catch errors with these
additional fixes, hence all the patches in this series were squashed then
tested with the reproducer for [1].
Overall, this series makes the following changes:
- Patch 1: Schedule SCO sock timeouts with delayed_work to avoid
inconsistent lock usage (removes SOFTIRQs from SCO)
- Patch 2: Avoid a circular dependency between hci_dev_lock and
lock_sock (enables the switch to lock_sock)
- Patch 3: Switch to lock_sock in SCO now that SOFTIRQs and potential
deadlocks are removed
- Patch 4: Serialize calls to sco_sock_{set,clear}_timer
- Patch 5: Switch to lock_sock in RFCOMM
- Patch 6: fix SCO socket killing
v4 -> v5:
- Renamed the delayed_work variable, moved checks for sco_pi(sk)->conn
into sco_sock_{clear,set}_timer, as suggested by Luiz Augusto von Dentz
and Marcel Holtmann.
- Added check for conn->sk in sco_sock_timeout, accompanied by a
sock_hold to avoid UAF errors.
- Added check to flush work items before freeing conn.
- Avoid a circular dependency between hci_dev_lock and lock_sock.
- Switch to lock_sock in SCO, as suggested by Marcel Holtmann.
- Serial calls to sco_sock_{set,clear}_timer.
- Switch to lock_sock in RFCOMM, as suggested by Marcel Holtmann.
- Add a fix for SCO socket killing.
v3 -> v4:
- Switch to using delayed_work to schedule SCO sock timeouts instead
of using local_bh_disable. As suggested by Luiz Augusto von Dentz.
v2 -> v3:
- Split SCO and RFCOMM code changes, as suggested by Luiz Augusto von
Dentz.
- Simplify local bh disabling in SCO by using local_bh_disable/enable
inside sco_chan_del since local_bh_disable/enable pairs are reentrant.
v1 -> v2:
- Instead of pulling out the clean-up code out from sco_chan_del and
using it directly in sco_conn_del, disable local softirqs for relevant
sections.
- Disable local softirqs more thoroughly for instances of
bh_lock_sock/bh_lock_sock_nested in the bluetooth subsystem.
Specifically, the calls in af_bluetooth.c and rfcomm/sock.c are now made
with local softirqs disabled as well.
Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Link: https://syzkaller.appspot.com/bug?extid=66264bf2fd0476be7e6c [2]
Link: https://syzkaller.appspot.com/text?tag=CrashReport&x=172d819a300000 [3]
Link: https://syzkaller.appspot.com/bug?id=e1bf7ba90d8dafcf318666192aba1cfd65507377 [4]
Best wishes,
Desmond
Desmond Cheong Zhi Xi (6):
Bluetooth: schedule SCO timeouts with delayed_work
Bluetooth: avoid circular locks in sco_sock_connect
Bluetooth: switch to lock_sock in SCO
Bluetooth: serialize calls to sco_sock_{set,clear}_timer
Bluetooth: switch to lock_sock in RFCOMM
Bluetooth: fix repeated calls to sco_sock_kill
net/bluetooth/rfcomm/sock.c | 8 +--
net/bluetooth/sco.c | 107 +++++++++++++++++++++---------------
2 files changed, 66 insertions(+), 49 deletions(-)
--
2.25.1
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-04 15:48:39
struct sock.sk_timer should be used as a sock cleanup timer. However,
SCO uses it to implement sock timeouts.
This causes issues because struct sock.sk_timer's callback is run in
an IRQ context, and the timer callback function sco_sock_timeout takes
a spin lock on the socket. However, other functions such as
sco_conn_del and sco_conn_ready take the spin lock with interrupts
enabled.
This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
lead to deadlocks as reported by Syzbot [1]:
CPU0
----
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
<Interrupt>
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
To fix this, we use delayed work to implement SCO sock timouts
instead. This allows us to avoid taking the spin lock on the socket in
an IRQ context, and corrects the misuse of struct sock.sk_timer.
As a note, cancel_delayed_work is used instead of
cancel_delayed_work_sync in sco_sock_set_timer and
sco_sock_clear_timer to avoid a deadlock. In the future, the call to
bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
synchronize with other functions using lock_sock. However, since
sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
the locked socket (in sco_connect and __sco_sock_close),
cancel_delayed_work_sync might cause them to sleep until an
sco_sock_timeout that has started finishes running. But
sco_sock_timeout would also sleep until it can grab the lock_sock.
Using cancel_delayed_work is fine because sco_sock_timeout does not
change from run to run, hence there is no functional difference
between:
1. waiting for a timeout to finish running before scheduling another
timeout
2. scheduling another timeout while a timeout is running.
Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
@@ -179,6 +205,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)bh_unlock_sock(sk);sco_sock_kill(sk);sock_put(sk);++/* Ensure no more work items will run before freeing conn. */+cancel_delayed_work_sync(&conn->timeout_work);}hcon->sco_data=NULL;
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=526413
---Test result---
Test Summary:
CheckPatch FAIL 1.96 seconds
GitLint FAIL 0.19 seconds
BuildKernel PASS 505.53 seconds
TestRunner: Setup PASS 345.25 seconds
TestRunner: l2cap-tester PASS 2.57 seconds
TestRunner: bnep-tester PASS 1.90 seconds
TestRunner: mgmt-tester PASS 30.34 seconds
TestRunner: rfcomm-tester PASS 2.08 seconds
TestRunner: sco-tester PASS 2.02 seconds
TestRunner: smp-tester FAIL 2.06 seconds
TestRunner: userchan-tester PASS 1.92 seconds
Details
##############################
Test: CheckPatch - FAIL - 1.96 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: fix repeated calls to sco_sock_kill
WARNING: Unknown commit id '4e1a720d0312', maybe rebased or not pulled?
#6:
In commit 4e1a720d0312 ("Bluetooth: avoid killing an already killed
WARNING: Unknown commit id '4e1a720d0312', maybe rebased or not pulled?
#34:
Fixes: 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket")
total: 0 errors, 2 warnings, 0 checks, 31 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 repeated calls to sco_sock_kill" 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 - FAIL - 0.19 seconds
Run gitlint with rule in .gitlint
Bluetooth: schedule SCO timeouts with delayed_work
42: B1 Line exceeds max length (87>80): "Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]"
Bluetooth: avoid circular locks in sco_sock_connect
109: B3 Line contains hard tab characters (\t): " sco_disconn_cfm():"
110: B3 Line contains hard tab characters (\t): " sco_conn_del():"
111: B3 Line contains hard tab characters (\t): " lock_sock(sk);"
##############################
Test: BuildKernel - PASS - 505.53 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 345.25 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 2.57 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 1.90 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - PASS - 30.34 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3
##############################
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 - 2.02 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - FAIL - 2.06 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
Failed Test Cases
SMP Client - SC Request 2 Failed 0.021 seconds
##############################
Test: TestRunner: userchan-tester - PASS - 1.92 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Date: 2021-08-05 19:07:03
Hi Desmond,
On Wed, Aug 4, 2021 at 8:48 AM Desmond Cheong Zhi Xi
[off-list ref] wrote:
quoted hunk
struct sock.sk_timer should be used as a sock cleanup timer. However,
SCO uses it to implement sock timeouts.
This causes issues because struct sock.sk_timer's callback is run in
an IRQ context, and the timer callback function sco_sock_timeout takes
a spin lock on the socket. However, other functions such as
sco_conn_del and sco_conn_ready take the spin lock with interrupts
enabled.
This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
lead to deadlocks as reported by Syzbot [1]:
CPU0
----
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
<Interrupt>
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
To fix this, we use delayed work to implement SCO sock timouts
instead. This allows us to avoid taking the spin lock on the socket in
an IRQ context, and corrects the misuse of struct sock.sk_timer.
As a note, cancel_delayed_work is used instead of
cancel_delayed_work_sync in sco_sock_set_timer and
sco_sock_clear_timer to avoid a deadlock. In the future, the call to
bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
synchronize with other functions using lock_sock. However, since
sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
the locked socket (in sco_connect and __sco_sock_close),
cancel_delayed_work_sync might cause them to sleep until an
sco_sock_timeout that has started finishes running. But
sco_sock_timeout would also sleep until it can grab the lock_sock.
Using cancel_delayed_work is fine because sco_sock_timeout does not
change from run to run, hence there is no functional difference
between:
1. waiting for a timeout to finish running before scheduling another
timeout
2. scheduling another timeout while a timeout is running.
Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
+ if (!sco_pi(sk)->conn)
+ return;
+ work = &sco_pi(sk)->conn->timeout_work;
+
BT_DBG("sock %p state %d", sk, sk->sk_state);
- sk_stop_timer(sk, &sk->sk_timer);
+ cancel_delayed_work(work);
}
/* ---- SCO connections ---- */
@@ -179,6 +205,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err) bh_unlock_sock(sk); sco_sock_kill(sk); sock_put(sk);++ /* Ensure no more work items will run before freeing conn. */+ cancel_delayed_work_sync(&conn->timeout_work); } hcon->sco_data = NULL;
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-09 04:04:40
On 6/8/21 3:06 am, Luiz Augusto von Dentz wrote:
Hi Desmond,
On Wed, Aug 4, 2021 at 8:48 AM Desmond Cheong Zhi Xi
[off-list ref] wrote:
quoted
struct sock.sk_timer should be used as a sock cleanup timer. However,
SCO uses it to implement sock timeouts.
This causes issues because struct sock.sk_timer's callback is run in
an IRQ context, and the timer callback function sco_sock_timeout takes
a spin lock on the socket. However, other functions such as
sco_conn_del and sco_conn_ready take the spin lock with interrupts
enabled.
This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
lead to deadlocks as reported by Syzbot [1]:
CPU0
----
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
<Interrupt>
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
To fix this, we use delayed work to implement SCO sock timouts
instead. This allows us to avoid taking the spin lock on the socket in
an IRQ context, and corrects the misuse of struct sock.sk_timer.
As a note, cancel_delayed_work is used instead of
cancel_delayed_work_sync in sco_sock_set_timer and
sco_sock_clear_timer to avoid a deadlock. In the future, the call to
bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
synchronize with other functions using lock_sock. However, since
sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
the locked socket (in sco_connect and __sco_sock_close),
cancel_delayed_work_sync might cause them to sleep until an
sco_sock_timeout that has started finishes running. But
sco_sock_timeout would also sleep until it can grab the lock_sock.
Using cancel_delayed_work is fine because sco_sock_timeout does not
change from run to run, hence there is no functional difference
between:
1. waiting for a timeout to finish running before scheduling another
timeout
2. scheduling another timeout while a timeout is running.
Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
Minor nitpick but I don't think using a dedicated variable here makes
much sense.
Thanks for the feedback, Luiz. Agreed, I can make the change in the next
version of the series after the other patches are reviewed.
Best wishes,
Desmond
+ if (!sco_pi(sk)->conn)
+ return;
+ work = &sco_pi(sk)->conn->timeout_work;
+
BT_DBG("sock %p state %d", sk, sk->sk_state);
- sk_stop_timer(sk, &sk->sk_timer);
+ cancel_delayed_work(work);
}
/* ---- SCO connections ---- */
@@ -179,6 +205,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err) bh_unlock_sock(sk); sco_sock_kill(sk); sock_put(sk);++ /* Ensure no more work items will run before freeing conn. */+ cancel_delayed_work_sync(&conn->timeout_work); } hcon->sco_data = NULL;
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Date: 2021-08-09 16:42:30
Hi Desmond,
On Sun, Aug 8, 2021 at 9:04 PM Desmond Cheong Zhi Xi
[off-list ref] wrote:
On 6/8/21 3:06 am, Luiz Augusto von Dentz wrote:
quoted
Hi Desmond,
On Wed, Aug 4, 2021 at 8:48 AM Desmond Cheong Zhi Xi
[off-list ref] wrote:
quoted
struct sock.sk_timer should be used as a sock cleanup timer. However,
SCO uses it to implement sock timeouts.
This causes issues because struct sock.sk_timer's callback is run in
an IRQ context, and the timer callback function sco_sock_timeout takes
a spin lock on the socket. However, other functions such as
sco_conn_del and sco_conn_ready take the spin lock with interrupts
enabled.
This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
lead to deadlocks as reported by Syzbot [1]:
CPU0
----
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
<Interrupt>
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
To fix this, we use delayed work to implement SCO sock timouts
instead. This allows us to avoid taking the spin lock on the socket in
an IRQ context, and corrects the misuse of struct sock.sk_timer.
As a note, cancel_delayed_work is used instead of
cancel_delayed_work_sync in sco_sock_set_timer and
sco_sock_clear_timer to avoid a deadlock. In the future, the call to
bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
synchronize with other functions using lock_sock. However, since
sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
the locked socket (in sco_connect and __sco_sock_close),
cancel_delayed_work_sync might cause them to sleep until an
sco_sock_timeout that has started finishes running. But
sco_sock_timeout would also sleep until it can grab the lock_sock.
Using cancel_delayed_work is fine because sco_sock_timeout does not
change from run to run, hence there is no functional difference
between:
1. waiting for a timeout to finish running before scheduling another
timeout
2. scheduling another timeout while a timeout is running.
Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
+ if (!sco_pi(sk)->conn)
+ return;
+ work = &sco_pi(sk)->conn->timeout_work;
+
BT_DBG("sock %p state %d", sk, sk->sk_state);
- sk_stop_timer(sk, &sk->sk_timer);
+ cancel_delayed_work(work);
}
/* ---- SCO connections ---- */
@@ -179,6 +205,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err) bh_unlock_sock(sk); sco_sock_kill(sk); sock_put(sk);++ /* Ensure no more work items will run before freeing conn. */+ cancel_delayed_work_sync(&conn->timeout_work); } hcon->sco_data = NULL;
@@ -243,44 +243,32 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,returnerr;}-staticintsco_connect(structsock*sk)+staticintsco_connect(structhci_dev*hdev,structsock*sk){structsco_conn*conn;structhci_conn*hcon;-structhci_dev*hdev;interr,type;BT_DBG("%pMR -> %pMR",&sco_pi(sk)->src,&sco_pi(sk)->dst);-hdev=hci_get_route(&sco_pi(sk)->dst,&sco_pi(sk)->src,BDADDR_BREDR);-if(!hdev)-return-EHOSTUNREACH;--hci_dev_lock(hdev);-if(lmp_esco_capable(hdev)&&!disable_esco)type=ESCO_LINK;elsetype=SCO_LINK;if(sco_pi(sk)->setting==BT_VOICE_TRANSPARENT&&-(!lmp_transp_capable(hdev)||!lmp_esco_capable(hdev))){-err=-EOPNOTSUPP;-gotodone;-}+(!lmp_transp_capable(hdev)||!lmp_esco_capable(hdev)))+return-EOPNOTSUPP;hcon=hci_connect_sco(hdev,type,&sco_pi(sk)->dst,sco_pi(sk)->setting);-if(IS_ERR(hcon)){-err=PTR_ERR(hcon);-gotodone;-}+if(IS_ERR(hcon))+returnPTR_ERR(hcon);conn=sco_conn_add(hcon);if(!conn){hci_conn_drop(hcon);-err=-ENOMEM;-gotodone;+return-ENOMEM;}/* Update source addr of the socket */
@@ -288,7 +276,7 @@ static int sco_connect(struct sock *sk)err=sco_chan_add(conn,sk,NULL);if(err)-gotodone;+returnerr;if(hcon->state==BT_CONNECTED){sco_sock_clear_timer(sk);
@@ -298,9 +286,6 @@ static int sco_connect(struct sock *sk)sco_sock_set_timer(sk,sk->sk_sndtimeo);}-done:-hci_dev_unlock(hdev);-hci_dev_put(hdev);returnerr;}
@@ -595,6 +580,7 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen{structsockaddr_sco*sa=(structsockaddr_sco*)addr;structsock*sk=sock->sk;+structhci_dev*hdev;interr;BT_DBG("sk %p",sk);
@@ -609,12 +595,19 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alenif(sk->sk_type!=SOCK_SEQPACKET)return-EINVAL;+hdev=hci_get_route(&sa->sco_bdaddr,&sco_pi(sk)->src,BDADDR_BREDR);+if(!hdev)+return-EHOSTUNREACH;+hci_dev_lock(hdev);+lock_sock(sk);/* Set destination address and psm */bacpy(&sco_pi(sk)->dst,&sa->sco_bdaddr);-err=sco_connect(sk);+err=sco_connect(hdev,sk);+hci_dev_unlock(hdev);+hci_dev_put(hdev);if(err)gotodone;
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-04 15:48:51
Since sco_sock_timeout is now scheduled using delayed work, it is no
longer run in SOFTIRQ context. Hence bh_lock_sock is no longer
necessary in SCO to synchronise between user contexts and SOFTIRQ
processing.
As such, calls to bh_lock_sock should be replaced with lock_sock to
synchronize with other concurrent processes that use lock_sock.
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-04 15:49:21
Currently, calls to sco_sock_set_timer are made under the locked
socket, but this does not apply to all calls to sco_sock_clear_timer.
Both sco_sock_{set,clear}_timer should be serialized by lock_sock to
prevent unexpected concurrent clearing/setting of timers.
Additionally, since sco_pi(sk)->conn is only cleared under the locked
socket, this change allows us to avoid races between
sco_sock_clear_timer and the call to kfree(conn) in sco_conn_del.
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -459,8 +459,8 @@ static void __sco_sock_close(struct sock *sk)/* Must be called on unlocked socket. */staticvoidsco_sock_close(structsock*sk){-sco_sock_clear_timer(sk);lock_sock(sk);+sco_sock_clear_timer(sk);__sco_sock_close(sk);release_sock(sk);sco_sock_kill(sk);
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-04 15:49:24
Other than rfcomm_sk_state_change and rfcomm_connect_ind, functions in
RFCOMM use lock_sock to lock the socket.
Since bh_lock_sock and spin_lock_bh do not provide synchronization
with lock_sock, these calls should be changed to lock_sock.
This is now safe to do because packet processing is now done in a
workqueue instead of a tasklet, so bh_lock_sock/spin_lock_bh are no
longer necessary to synchronise between user contexts and SOFTIRQ
processing.
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/rfcomm/sock.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -70,7 +70,7 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)BT_DBG("dlc %p state %ld err %d",d,d->state,err);-spin_lock_bh(&sk->sk_lock.slock);+lock_sock(sk);if(err)sk->sk_err=err;
@@ -91,7 +91,7 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)sk->sk_state_change(sk);}-spin_unlock_bh(&sk->sk_lock.slock);+release_sock(sk);if(parent&&sock_flag(sk,SOCK_ZAPPED)){/* We have to drop DLC lock here, otherwise
@@ -974,7 +974,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *if(!parent)return0;-bh_lock_sock(parent);+lock_sock(parent);/* Check for backlog size */if(sk_acceptq_is_full(parent)){
From: Desmond Cheong Zhi Xi <hidden> Date: 2021-08-04 15:50:04
In commit 4e1a720d0312 ("Bluetooth: avoid killing an already killed
socket"), a check was added to sco_sock_kill to skip killing a socket
if the SOCK_DEAD flag was set.
This was done after a trace for a use-after-free bug showed that the
same sock pointer was being killed twice.
Unfortunately, this check prevents sco_sock_kill from running on any
socket. sco_sock_kill kills a socket only if it's zapped and orphaned,
however sock_orphan announces that the socket is dead before detaching
it. i.e., orphaned sockets have the SOCK_DEAD flag set.
To fix this, we remove the check for SOCK_DEAD, and avoid repeated
calls to sco_sock_kill by removing incorrect calls in:
1. sco_sock_timeout. The socket should not be killed on timeout as
further processing is expected to be done. For example,
sco_sock_connect sets the timer then waits for the socket to be
connected or for an error to be returned.
2. sco_conn_del. This function should clean up resources for the
connection, but the socket itself should be cleaned up in
sco_sock_release.
3. sco_sock_close. Calls to sco_sock_close in sco_sock_cleanup_listen
and sco_sock_release are followed by sco_sock_kill. Hence the
duplicated call should be removed.
Fixes: 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket")
Signed-off-by: Desmond Cheong Zhi Xi <redacted>
---
net/bluetooth/sco.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
@@ -203,7 +201,6 @@ static void sco_conn_del(struct hci_conn *hcon, int err)sco_sock_clear_timer(sk);sco_chan_del(sk,err);release_sock(sk);-sco_sock_kill(sk);sock_put(sk);/* Ensure no more work items will run before freeing conn. */