| Message ID | e9e2738d-78a7-1def-a86f-937951d139a7@i-love.sakura.ne.jp |
|---|---|
| State | Accepted |
| Commit | e04480920d1eec9c061841399aa6f35b6f987d8b |
| Headers | show |
| Series | [v4] Bluetooth: defer cleanup of resources in hci_unregister_dev() | expand |
Hi Tetsuo, On Wed, Aug 4, 2021 at 3:27 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > syzbot is hitting might_sleep() warning at hci_sock_dev_event() > due to calling lock_sock() with rw spinlock held [1]. > > It seems that history of this locking problem is a trial and error. > > Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in > hci_sock_dev_event()") in 2.6.21-rc4 changed bh_lock_sock() to lock_sock() > as an attempt to fix lockdep warning. > > Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in > hci_sock_dev_event().") in 2.6.22-rc2 changed lock_sock() to > local_bh_disable() + bh_lock_sock_nested() as an attempt to fix > sleep in atomic context warning. > > Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from > hci_sock.c") in 3.3-rc1 removed local_bh_disable(). > > Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF > of hdev object") in 5.13-rc5 again changed bh_lock_sock_nested() to > lock_sock() as an attempt to fix CVE-2021-3573. > > This difficulty comes from current implementation that > hci_sock_dev_event(HCI_DEV_UNREG) is responsible for dropping all > references from sockets because hci_unregister_dev() immediately reclaims > resources as soon as returning from hci_sock_dev_event(HCI_DEV_UNREG). > But the history suggests that hci_sock_dev_event(HCI_DEV_UNREG) was not > doing what it should do. > > Therefore, instead of trying to detach sockets from device, let's accept > not detaching sockets from device at hci_sock_dev_event(HCI_DEV_UNREG), > by moving actual cleanup of resources from hci_unregister_dev() to > hci_cleanup_dev() which is called by bt_host_release() when all references > to this unregistered device (which is a kobject) are gone. > > Since hci_sock_dev_event(HCI_DEV_UNREG) no longer resets hci_pi(sk)->hdev, > we need to check whether this device was unregistered and return an error > based on HCI_UNREGISTER flag. There might be subtle behavioral difference > in "monitor the hdev" functionality; please report if you found something > went wrong due to this patch. > > Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1] > Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object") > --- > v4: Don't touch "monitor the hdev" functions. > > v3: Unexport hci_unregister_dev because only bt_host_release calls it > and both hci_core.o and hci_sysfs.o are combined into bluetooth.o. > > Rename hci_release_dev to hci_cleanup_dev and call it only when > HCI_UNREGISTER was set, for syzbot > <syzbot+47c6d0efbb7fe2f7a5b8@syzkaller.appspotmail.com> is reporting > that bt_host_release might be called without hci_register_dev and > hci_unregister_dev. > > Return -EPIPE instead of 0 if HCI_UNREGISTER was set, in order to > make sure that userspace finds that the device was unregistred. > > v2: Add hci_release_dev and make bt_host_release call it instead of making > bt_host_release public. > > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_core.c | 16 +++++------ > net/bluetooth/hci_sock.c | 49 +++++++++++++++++++++----------- > net/bluetooth/hci_sysfs.c | 3 ++ > 4 files changed, 45 insertions(+), 24 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index a53e94459ecd..db4312e44d47 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -1230,6 +1230,7 @@ struct hci_dev *hci_alloc_dev(void); > void hci_free_dev(struct hci_dev *hdev); > int hci_register_dev(struct hci_dev *hdev); > void hci_unregister_dev(struct hci_dev *hdev); > +void hci_cleanup_dev(struct hci_dev *hdev); > int hci_suspend_dev(struct hci_dev *hdev); > int hci_resume_dev(struct hci_dev *hdev); > int hci_reset_dev(struct hci_dev *hdev); > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 2560ed2f144d..e1a545c8a69f 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -3996,14 +3996,10 @@ EXPORT_SYMBOL(hci_register_dev); > /* Unregister HCI device */ > void hci_unregister_dev(struct hci_dev *hdev) > { > - int id; > - > BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus); > > hci_dev_set_flag(hdev, HCI_UNREGISTER); > > - id = hdev->id; > - > write_lock(&hci_dev_list_lock); > list_del(&hdev->list); > write_unlock(&hci_dev_list_lock); > @@ -4038,7 +4034,14 @@ void hci_unregister_dev(struct hci_dev *hdev) > } > > device_del(&hdev->dev); > + /* Actual cleanup is deferred until hci_cleanup_dev(). */ > + hci_dev_put(hdev); > +} > +EXPORT_SYMBOL(hci_unregister_dev); > > +/* Cleanup HCI device */ > +void hci_cleanup_dev(struct hci_dev *hdev) > +{ > debugfs_remove_recursive(hdev->debugfs); > kfree_const(hdev->hw_info); > kfree_const(hdev->fw_info); > @@ -4063,11 +4066,8 @@ void hci_unregister_dev(struct hci_dev *hdev) > hci_blocked_keys_clear(hdev); > hci_dev_unlock(hdev); > > - hci_dev_put(hdev); > - > - ida_simple_remove(&hci_index_ida, id); > + ida_simple_remove(&hci_index_ida, hdev->id); > } > -EXPORT_SYMBOL(hci_unregister_dev); > > /* Suspend HCI device */ > int hci_suspend_dev(struct hci_dev *hdev) > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c > index b04a5a02ecf3..f1128c2134f0 100644 > --- a/net/bluetooth/hci_sock.c > +++ b/net/bluetooth/hci_sock.c > @@ -59,6 +59,17 @@ struct hci_pinfo { > char comm[TASK_COMM_LEN]; > }; > > +static struct hci_dev *hci_hdev_from_sock(struct sock *sk) > +{ > + struct hci_dev *hdev = hci_pi(sk)->hdev; > + > + if (!hdev) > + return ERR_PTR(-EBADFD); > + if (hci_dev_test_flag(hdev, HCI_UNREGISTER)) > + return ERR_PTR(-EPIPE); > + return hdev; > +} > + > void hci_sock_set_flag(struct sock *sk, int nr) > { > set_bit(nr, &hci_pi(sk)->flags); > @@ -759,19 +770,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) > if (event == HCI_DEV_UNREG) { > struct sock *sk; > > - /* Detach sockets from device */ > + /* Wake up sockets using this dead device */ > read_lock(&hci_sk_list.lock); > sk_for_each(sk, &hci_sk_list.head) { > - lock_sock(sk); > if (hci_pi(sk)->hdev == hdev) { > - hci_pi(sk)->hdev = NULL; > sk->sk_err = EPIPE; > - sk->sk_state = BT_OPEN; > sk->sk_state_change(sk); > - > - hci_dev_put(hdev); > } > - release_sock(sk); > } > read_unlock(&hci_sk_list.lock); > } > @@ -930,10 +935,10 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg) > static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd, > unsigned long arg) > { > - struct hci_dev *hdev = hci_pi(sk)->hdev; > + struct hci_dev *hdev = hci_hdev_from_sock(sk); > > - if (!hdev) > - return -EBADFD; > + if (IS_ERR(hdev)) > + return PTR_ERR(hdev); > > if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) > return -EBUSY; > @@ -1103,6 +1108,18 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, > > lock_sock(sk); > > + /* Allow detaching from dead device and attaching to alive device, if > + * the caller wants to re-bind (instead of close) this socket in > + * response to hci_sock_dev_event(HCI_DEV_UNREG) notification. > + */ > + hdev = hci_pi(sk)->hdev; > + if (hdev && hci_dev_test_flag(hdev, HCI_UNREGISTER)) { > + hci_pi(sk)->hdev = NULL; > + sk->sk_state = BT_OPEN; > + hci_dev_put(hdev); > + } > + hdev = NULL; > + > if (sk->sk_state == BT_BOUND) { > err = -EALREADY; > goto done; > @@ -1379,9 +1396,9 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr, > > lock_sock(sk); > > - hdev = hci_pi(sk)->hdev; > - if (!hdev) { > - err = -EBADFD; > + hdev = hci_hdev_from_sock(sk); > + if (IS_ERR(hdev)) { > + err = PTR_ERR(hdev); > goto done; > } > > @@ -1743,9 +1760,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg, > goto done; > } > > - hdev = hci_pi(sk)->hdev; > - if (!hdev) { > - err = -EBADFD; > + hdev = hci_hdev_from_sock(sk); > + if (IS_ERR(hdev)) { > + err = PTR_ERR(hdev); > goto done; > } > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c > index 9874844a95a9..b69d88b88d2e 100644 > --- a/net/bluetooth/hci_sysfs.c > +++ b/net/bluetooth/hci_sysfs.c > @@ -83,6 +83,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn) > static void bt_host_release(struct device *dev) > { > struct hci_dev *hdev = to_hci_dev(dev); > + > + if (hci_dev_test_flag(hdev, HCI_UNREGISTER)) > + hci_cleanup_dev(hdev); > kfree(hdev); > module_put(THIS_MODULE); > } > -- > 2.18.4 It doesn't apply to bluetooth-next, could you please rebase it. -- Luiz Augusto von Dentz
On Thu, Aug 5, 2021 at 11:19 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > It doesn't apply to bluetooth-next, could you please rebase it. If you're ok with the patch, I think it should go into 5.14 - and be marked for stable. The locking right now is very wrong, and causes syzbot warnings and probably keeps people from finding other things. And we can't just revert the incorrect locking change, because it was a security issue. So this should go in the current release cycle. I agree it's a bit scary - not because I think the patch is all that complicated, but because this area has clearly had issues and is subtle - but I'd rather do it asap and get reports on it early than delay it further. Linus
Hi Linus, On Thu, Aug 5, 2021 at 11:45 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, Aug 5, 2021 at 11:19 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > It doesn't apply to bluetooth-next, could you please rebase it. > > If you're ok with the patch, I think it should go into 5.14 - and be > marked for stable. > > The locking right now is very wrong, and causes syzbot warnings and > probably keeps people from finding other things. > > And we can't just revert the incorrect locking change, because it was > a security issue. > > So this should go in the current release cycle. I agree it's a bit > scary - not because I think the patch is all that complicated, but > because this area has clearly had issues and is subtle - but I'd > rather do it asap and get reports on it early than delay it further. Alright so we are skipping bluetooth-next then, I thought of using bluetooth-next for further testing these changes but I agree this should go into 5.14 so Im fine if you take it straight to your tree: Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> -- Luiz Augusto von Dentz
On Thu, Aug 5, 2021 at 11:59 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Alright so we are skipping bluetooth-next then, I thought of using > bluetooth-next for further testing these changes but I agree this > should go into 5.14 so Im fine if you take it straight to your tree: Turned your sign-off into an acked-by, since the patch came directly from Tetsuo to me, but it's commit e04480920d1e ("Bluetooth: defer cleanup of resources in hci_unregister_dev()") in my tree now. Let's hope this closes the issue, but keep our ears to the ground in case something pops up. Tetsuo, thanks for driving this. Linus
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index a53e94459ecd..db4312e44d47 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -1230,6 +1230,7 @@ struct hci_dev *hci_alloc_dev(void); void hci_free_dev(struct hci_dev *hdev); int hci_register_dev(struct hci_dev *hdev); void hci_unregister_dev(struct hci_dev *hdev); +void hci_cleanup_dev(struct hci_dev *hdev); int hci_suspend_dev(struct hci_dev *hdev); int hci_resume_dev(struct hci_dev *hdev); int hci_reset_dev(struct hci_dev *hdev); diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 2560ed2f144d..e1a545c8a69f 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3996,14 +3996,10 @@ EXPORT_SYMBOL(hci_register_dev); /* Unregister HCI device */ void hci_unregister_dev(struct hci_dev *hdev) { - int id; - BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus); hci_dev_set_flag(hdev, HCI_UNREGISTER); - id = hdev->id; - write_lock(&hci_dev_list_lock); list_del(&hdev->list); write_unlock(&hci_dev_list_lock); @@ -4038,7 +4034,14 @@ void hci_unregister_dev(struct hci_dev *hdev) } device_del(&hdev->dev); + /* Actual cleanup is deferred until hci_cleanup_dev(). */ + hci_dev_put(hdev); +} +EXPORT_SYMBOL(hci_unregister_dev); +/* Cleanup HCI device */ +void hci_cleanup_dev(struct hci_dev *hdev) +{ debugfs_remove_recursive(hdev->debugfs); kfree_const(hdev->hw_info); kfree_const(hdev->fw_info); @@ -4063,11 +4066,8 @@ void hci_unregister_dev(struct hci_dev *hdev) hci_blocked_keys_clear(hdev); hci_dev_unlock(hdev); - hci_dev_put(hdev); - - ida_simple_remove(&hci_index_ida, id); + ida_simple_remove(&hci_index_ida, hdev->id); } -EXPORT_SYMBOL(hci_unregister_dev); /* Suspend HCI device */ int hci_suspend_dev(struct hci_dev *hdev) diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c index b04a5a02ecf3..f1128c2134f0 100644 --- a/net/bluetooth/hci_sock.c +++ b/net/bluetooth/hci_sock.c @@ -59,6 +59,17 @@ struct hci_pinfo { char comm[TASK_COMM_LEN]; }; +static struct hci_dev *hci_hdev_from_sock(struct sock *sk) +{ + struct hci_dev *hdev = hci_pi(sk)->hdev; + + if (!hdev) + return ERR_PTR(-EBADFD); + if (hci_dev_test_flag(hdev, HCI_UNREGISTER)) + return ERR_PTR(-EPIPE); + return hdev; +} + void hci_sock_set_flag(struct sock *sk, int nr) { set_bit(nr, &hci_pi(sk)->flags); @@ -759,19 +770,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event) if (event == HCI_DEV_UNREG) { struct sock *sk; - /* Detach sockets from device */ + /* Wake up sockets using this dead device */ read_lock(&hci_sk_list.lock); sk_for_each(sk, &hci_sk_list.head) { - lock_sock(sk); if (hci_pi(sk)->hdev == hdev) { - hci_pi(sk)->hdev = NULL; sk->sk_err = EPIPE; - sk->sk_state = BT_OPEN; sk->sk_state_change(sk); - - hci_dev_put(hdev); } - release_sock(sk); } read_unlock(&hci_sk_list.lock); } @@ -930,10 +935,10 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg) static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd, unsigned long arg) { - struct hci_dev *hdev = hci_pi(sk)->hdev; + struct hci_dev *hdev = hci_hdev_from_sock(sk); - if (!hdev) - return -EBADFD; + if (IS_ERR(hdev)) + return PTR_ERR(hdev); if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) return -EBUSY; @@ -1103,6 +1108,18 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, lock_sock(sk); + /* Allow detaching from dead device and attaching to alive device, if + * the caller wants to re-bind (instead of close) this socket in + * response to hci_sock_dev_event(HCI_DEV_UNREG) notification. + */ + hdev = hci_pi(sk)->hdev; + if (hdev && hci_dev_test_flag(hdev, HCI_UNREGISTER)) { + hci_pi(sk)->hdev = NULL; + sk->sk_state = BT_OPEN; + hci_dev_put(hdev); + } + hdev = NULL; + if (sk->sk_state == BT_BOUND) { err = -EALREADY; goto done; @@ -1379,9 +1396,9 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr, lock_sock(sk); - hdev = hci_pi(sk)->hdev; - if (!hdev) { - err = -EBADFD; + hdev = hci_hdev_from_sock(sk); + if (IS_ERR(hdev)) { + err = PTR_ERR(hdev); goto done; } @@ -1743,9 +1760,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg, goto done; } - hdev = hci_pi(sk)->hdev; - if (!hdev) { - err = -EBADFD; + hdev = hci_hdev_from_sock(sk); + if (IS_ERR(hdev)) { + err = PTR_ERR(hdev); goto done; } diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c index 9874844a95a9..b69d88b88d2e 100644 --- a/net/bluetooth/hci_sysfs.c +++ b/net/bluetooth/hci_sysfs.c @@ -83,6 +83,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn) static void bt_host_release(struct device *dev) { struct hci_dev *hdev = to_hci_dev(dev); + + if (hci_dev_test_flag(hdev, HCI_UNREGISTER)) + hci_cleanup_dev(hdev); kfree(hdev); module_put(THIS_MODULE); }
syzbot is hitting might_sleep() warning at hci_sock_dev_event() due to calling lock_sock() with rw spinlock held [1]. It seems that history of this locking problem is a trial and error. Commit b40df5743ee8aed8 ("[PATCH] bluetooth: fix socket locking in hci_sock_dev_event()") in 2.6.21-rc4 changed bh_lock_sock() to lock_sock() as an attempt to fix lockdep warning. Then, commit 4ce61d1c7a8ef4c1 ("[BLUETOOTH]: Fix locking in hci_sock_dev_event().") in 2.6.22-rc2 changed lock_sock() to local_bh_disable() + bh_lock_sock_nested() as an attempt to fix sleep in atomic context warning. Then, commit 4b5dd696f81b210c ("Bluetooth: Remove local_bh_disable() from hci_sock.c") in 3.3-rc1 removed local_bh_disable(). Then, commit e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object") in 5.13-rc5 again changed bh_lock_sock_nested() to lock_sock() as an attempt to fix CVE-2021-3573. This difficulty comes from current implementation that hci_sock_dev_event(HCI_DEV_UNREG) is responsible for dropping all references from sockets because hci_unregister_dev() immediately reclaims resources as soon as returning from hci_sock_dev_event(HCI_DEV_UNREG). But the history suggests that hci_sock_dev_event(HCI_DEV_UNREG) was not doing what it should do. Therefore, instead of trying to detach sockets from device, let's accept not detaching sockets from device at hci_sock_dev_event(HCI_DEV_UNREG), by moving actual cleanup of resources from hci_unregister_dev() to hci_cleanup_dev() which is called by bt_host_release() when all references to this unregistered device (which is a kobject) are gone. Since hci_sock_dev_event(HCI_DEV_UNREG) no longer resets hci_pi(sk)->hdev, we need to check whether this device was unregistered and return an error based on HCI_UNREGISTER flag. There might be subtle behavioral difference in "monitor the hdev" functionality; please report if you found something went wrong due to this patch. Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1] Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object") --- v4: Don't touch "monitor the hdev" functions. v3: Unexport hci_unregister_dev because only bt_host_release calls it and both hci_core.o and hci_sysfs.o are combined into bluetooth.o. Rename hci_release_dev to hci_cleanup_dev and call it only when HCI_UNREGISTER was set, for syzbot <syzbot+47c6d0efbb7fe2f7a5b8@syzkaller.appspotmail.com> is reporting that bt_host_release might be called without hci_register_dev and hci_unregister_dev. Return -EPIPE instead of 0 if HCI_UNREGISTER was set, in order to make sure that userspace finds that the device was unregistred. v2: Add hci_release_dev and make bt_host_release call it instead of making bt_host_release public. include/net/bluetooth/hci_core.h | 1 + net/bluetooth/hci_core.c | 16 +++++------ net/bluetooth/hci_sock.c | 49 +++++++++++++++++++++----------- net/bluetooth/hci_sysfs.c | 3 ++ 4 files changed, 45 insertions(+), 24 deletions(-)