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