* [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout
@ 2022-03-25 3:30 Ying Hsu
2022-03-25 4:06 ` bluez.test.bot
2022-03-25 18:50 ` [PATCH] " Luiz Augusto von Dentz
0 siblings, 2 replies; 5+ messages in thread
From: Ying Hsu @ 2022-03-25 3:30 UTC (permalink / raw)
To: marcel
Cc: chromeos-bluetooth-upstreaming, Ying Hsu, Joseph Hwang,
David S. Miller, Jakub Kicinski, Johan Hedberg,
Luiz Augusto von Dentz, Paolo Abeni, linux-bluetooth,
linux-kernel, netdev
Connecting the same socket twice consecutively in sco_sock_connect()
could lead to a race condition where two sco_conn objects are created
but only one is associated with the socket. If the socket is closed
before the SCO connection is established, the timer associated with the
dangling sco_conn object won't be canceled. As the sock object is being
freed, the use-after-free problem happens when the timer callback
function sco_sock_timeout() accesses the socket. Here's the call trace:
dump_stack+0x107/0x163
? refcount_inc+0x1c/
print_address_description.constprop.0+0x1c/0x47e
? refcount_inc+0x1c/0x7b
kasan_report+0x13a/0x173
? refcount_inc+0x1c/0x7b
check_memory_region+0x132/0x139
refcount_inc+0x1c/0x7b
sco_sock_timeout+0xb2/0x1ba
process_one_work+0x739/0xbd1
? cancel_delayed_work+0x13f/0x13f
? __raw_spin_lock_init+0xf0/0xf0
? to_kthread+0x59/0x85
worker_thread+0x593/0x70e
kthread+0x346/0x35a
? drain_workqueue+0x31a/0x31a
? kthread_bind+0x4b/0x4b
ret_from_fork+0x1f/0x30
Signed-off-by: Ying Hsu <yinghsu@chromium.org>
Reviewed-by: Joseph Hwang <josephsih@chromium.org>
---
Tested this commit using a C reproducer on qemu-x86_64 for 8 hours.
net/bluetooth/sco.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 8eabf41b2993..380c63194736 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -574,19 +574,24 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
addr->sa_family != AF_BLUETOOTH)
return -EINVAL;
- if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
- return -EBADFD;
+ lock_sock(sk);
+ if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
+ err = -EBADFD;
+ goto done;
+ }
- if (sk->sk_type != SOCK_SEQPACKET)
- return -EINVAL;
+ if (sk->sk_type != SOCK_SEQPACKET) {
+ err = -EINVAL;
+ goto done;
+ }
hdev = hci_get_route(&sa->sco_bdaddr, &sco_pi(sk)->src, BDADDR_BREDR);
- if (!hdev)
- return -EHOSTUNREACH;
+ if (!hdev) {
+ err = -EHOSTUNREACH;
+ goto done;
+ }
hci_dev_lock(hdev);
- lock_sock(sk);
-
/* Set destination address and psm */
bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
--
2.35.1.1021.g381101b075-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout
2022-03-25 3:30 [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout Ying Hsu
@ 2022-03-25 4:06 ` bluez.test.bot
2022-03-25 18:50 ` [PATCH] " Luiz Augusto von Dentz
1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2022-03-25 4:06 UTC (permalink / raw)
To: linux-bluetooth, yinghsu
[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]
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=626220
---Test result---
Test Summary:
CheckPatch PASS 1.64 seconds
GitLint PASS 1.01 seconds
SubjectPrefix PASS 0.82 seconds
BuildKernel PASS 34.79 seconds
BuildKernel32 PASS 31.51 seconds
Incremental Build with patchesPASS 41.66 seconds
TestRunner: Setup PASS 529.53 seconds
TestRunner: l2cap-tester PASS 16.42 seconds
TestRunner: bnep-tester PASS 6.76 seconds
TestRunner: mgmt-tester PASS 108.12 seconds
TestRunner: rfcomm-tester PASS 8.97 seconds
TestRunner: sco-tester PASS 8.37 seconds
TestRunner: smp-tester PASS 8.39 seconds
TestRunner: userchan-tester PASS 6.94 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout
2022-03-25 3:30 [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout Ying Hsu
2022-03-25 4:06 ` bluez.test.bot
@ 2022-03-25 18:50 ` Luiz Augusto von Dentz
2022-03-26 6:31 ` Ying Hsu
1 sibling, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2022-03-25 18:50 UTC (permalink / raw)
To: Ying Hsu
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, Joseph Hwang,
David S. Miller, Jakub Kicinski, Johan Hedberg, Paolo Abeni,
linux-bluetooth, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Ying,
On Thu, Mar 24, 2022 at 8:31 PM Ying Hsu <yinghsu@chromium.org> wrote:
>
> Connecting the same socket twice consecutively in sco_sock_connect()
> could lead to a race condition where two sco_conn objects are created
> but only one is associated with the socket. If the socket is closed
> before the SCO connection is established, the timer associated with the
> dangling sco_conn object won't be canceled. As the sock object is being
> freed, the use-after-free problem happens when the timer callback
> function sco_sock_timeout() accesses the socket. Here's the call trace:
>
> dump_stack+0x107/0x163
> ? refcount_inc+0x1c/
> print_address_description.constprop.0+0x1c/0x47e
> ? refcount_inc+0x1c/0x7b
> kasan_report+0x13a/0x173
> ? refcount_inc+0x1c/0x7b
> check_memory_region+0x132/0x139
> refcount_inc+0x1c/0x7b
> sco_sock_timeout+0xb2/0x1ba
> process_one_work+0x739/0xbd1
> ? cancel_delayed_work+0x13f/0x13f
> ? __raw_spin_lock_init+0xf0/0xf0
> ? to_kthread+0x59/0x85
> worker_thread+0x593/0x70e
> kthread+0x346/0x35a
> ? drain_workqueue+0x31a/0x31a
> ? kthread_bind+0x4b/0x4b
> ret_from_fork+0x1f/0x30
>
> Signed-off-by: Ying Hsu <yinghsu@chromium.org>
> Reviewed-by: Joseph Hwang <josephsih@chromium.org>
> ---
> Tested this commit using a C reproducer on qemu-x86_64 for 8 hours.
We should probably add a link or something to the reproducer then, was
it syzbot? It does have some instructions on how to link its issues.
> net/bluetooth/sco.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 8eabf41b2993..380c63194736 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -574,19 +574,24 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
> addr->sa_family != AF_BLUETOOTH)
> return -EINVAL;
>
> - if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> - return -EBADFD;
> + lock_sock(sk);
> + if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
> + err = -EBADFD;
> + goto done;
> + }
>
> - if (sk->sk_type != SOCK_SEQPACKET)
> - return -EINVAL;
> + if (sk->sk_type != SOCK_SEQPACKET) {
> + err = -EINVAL;
> + goto done;
> + }
>
> hdev = hci_get_route(&sa->sco_bdaddr, &sco_pi(sk)->src, BDADDR_BREDR);
> - if (!hdev)
> - return -EHOSTUNREACH;
> + if (!hdev) {
> + err = -EHOSTUNREACH;
> + goto done;
> + }
> hci_dev_lock(hdev);
>
> - lock_sock(sk);
> -
Also are we sure we are not introducing a locking hierarchy problem
here? Previously we had hci_dev_lock then sock_lock now it is the
opposite, or perhaps we never want to have them at the same time?
> /* Set destination address and psm */
> bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
>
> --
> 2.35.1.1021.g381101b075-goog
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout
2022-03-25 18:50 ` [PATCH] " Luiz Augusto von Dentz
@ 2022-03-26 6:31 ` Ying Hsu
0 siblings, 0 replies; 5+ messages in thread
From: Ying Hsu @ 2022-03-26 6:31 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, Joseph Hwang,
David S. Miller, Jakub Kicinski, Johan Hedberg, Paolo Abeni,
linux-bluetooth, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Luiz,
I compiled and ran the c-reproducer:
https://syzkaller.appspot.com/x/repro.c?x=152b93e8700000
I will add relevant links in the commit message. Thanks for the reminder.
While fixing the use-after-free problem , I also found a possible
deadlock in sco_sock_connect() and sco_sock_getsockopt() :
sco_sock_connect() {
hci_dev_lock(hdev);
lock_sock(sk);
}
sco_sock_getsockopt() {
lock_sock(sk);
case BT_CODEC:
hci_dev_lock(hdev);
}
So, adjusting the locking order in sco_sock_connect() can also avoid
the possible deadlock.
Ying
On Sat, Mar 26, 2022 at 2:50 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Ying,
>
> On Thu, Mar 24, 2022 at 8:31 PM Ying Hsu <yinghsu@chromium.org> wrote:
> >
> > Connecting the same socket twice consecutively in sco_sock_connect()
> > could lead to a race condition where two sco_conn objects are created
> > but only one is associated with the socket. If the socket is closed
> > before the SCO connection is established, the timer associated with the
> > dangling sco_conn object won't be canceled. As the sock object is being
> > freed, the use-after-free problem happens when the timer callback
> > function sco_sock_timeout() accesses the socket. Here's the call trace:
> >
> > dump_stack+0x107/0x163
> > ? refcount_inc+0x1c/
> > print_address_description.constprop.0+0x1c/0x47e
> > ? refcount_inc+0x1c/0x7b
> > kasan_report+0x13a/0x173
> > ? refcount_inc+0x1c/0x7b
> > check_memory_region+0x132/0x139
> > refcount_inc+0x1c/0x7b
> > sco_sock_timeout+0xb2/0x1ba
> > process_one_work+0x739/0xbd1
> > ? cancel_delayed_work+0x13f/0x13f
> > ? __raw_spin_lock_init+0xf0/0xf0
> > ? to_kthread+0x59/0x85
> > worker_thread+0x593/0x70e
> > kthread+0x346/0x35a
> > ? drain_workqueue+0x31a/0x31a
> > ? kthread_bind+0x4b/0x4b
> > ret_from_fork+0x1f/0x30
> >
> > Signed-off-by: Ying Hsu <yinghsu@chromium.org>
> > Reviewed-by: Joseph Hwang <josephsih@chromium.org>
> > ---
> > Tested this commit using a C reproducer on qemu-x86_64 for 8 hours.
>
> We should probably add a link or something to the reproducer then, was
> it syzbot? It does have some instructions on how to link its issues.
>
> > net/bluetooth/sco.c | 21 +++++++++++++--------
> > 1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> > index 8eabf41b2993..380c63194736 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -574,19 +574,24 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
> > addr->sa_family != AF_BLUETOOTH)
> > return -EINVAL;
> >
> > - if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> > - return -EBADFD;
> > + lock_sock(sk);
> > + if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
> > + err = -EBADFD;
> > + goto done;
> > + }
> >
> > - if (sk->sk_type != SOCK_SEQPACKET)
> > - return -EINVAL;
> > + if (sk->sk_type != SOCK_SEQPACKET) {
> > + err = -EINVAL;
> > + goto done;
> > + }
> >
> > hdev = hci_get_route(&sa->sco_bdaddr, &sco_pi(sk)->src, BDADDR_BREDR);
> > - if (!hdev)
> > - return -EHOSTUNREACH;
> > + if (!hdev) {
> > + err = -EHOSTUNREACH;
> > + goto done;
> > + }
> > hci_dev_lock(hdev);
> >
> > - lock_sock(sk);
> > -
>
> Also are we sure we are not introducing a locking hierarchy problem
> here? Previously we had hci_dev_lock then sock_lock now it is the
> opposite, or perhaps we never want to have them at the same time?
>
> > /* Set destination address and psm */
> > bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
> >
> > --
> > 2.35.1.1021.g381101b075-goog
> >
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout
2022-03-26 6:35 Ying Hsu
@ 2022-03-26 7:11 ` bluez.test.bot
0 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2022-03-26 7:11 UTC (permalink / raw)
To: linux-bluetooth, yinghsu
[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]
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=626494
---Test result---
Test Summary:
CheckPatch PASS 1.77 seconds
GitLint PASS 1.06 seconds
SubjectPrefix PASS 0.86 seconds
BuildKernel PASS 43.89 seconds
BuildKernel32 PASS 39.92 seconds
Incremental Build with patchesPASS 52.73 seconds
TestRunner: Setup PASS 686.83 seconds
TestRunner: l2cap-tester PASS 20.11 seconds
TestRunner: bnep-tester PASS 8.56 seconds
TestRunner: mgmt-tester PASS 135.33 seconds
TestRunner: rfcomm-tester PASS 11.01 seconds
TestRunner: sco-tester PASS 10.73 seconds
TestRunner: smp-tester PASS 10.60 seconds
TestRunner: userchan-tester PASS 8.91 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-26 7:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-25 3:30 [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout Ying Hsu
2022-03-25 4:06 ` bluez.test.bot
2022-03-25 18:50 ` [PATCH] " Luiz Augusto von Dentz
2022-03-26 6:31 ` Ying Hsu
2022-03-26 6:35 Ying Hsu
2022-03-26 7:11 ` bluez.test.bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.