| Message ID | 290fe7c7-c14f-8685-af09-419faa0e4d1f@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] Bluetooth: defer cleanup of resources in hci_unregister_dev() | expand |
On Mon, Aug 2, 2021 at 8:19 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > @@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb) > sk_for_each(sk, &hci_sk_list.head) { > struct sk_buff *nskb; > > - if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev) > + if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev) > continue; > > /* Don't send frame to the socket it came from */ I'm not convinced this is a good idea. Here we use hci_hdev_from_sock() outside the socket lock, so now that HCI_UNREGISTER test is not very logical. And it's positively stupid to start with a hdev, then walk the socket list to look up another hdev, and then check "was it the original hdev, but not unregistered"? In other words - if you care about HCI_UNREGISTER state, it should be *above* the whole loop. Not inside of it. So I think this test should remain just that hci_pi(sk)->hdev != hdev and if HCI_UNREGISTER is relevant - and I don't think it is - it should have been done earlier. > @@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk) > > hdr = skb_push(skb, HCI_MON_HDR_SIZE); > hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN); > - if (hci_pi(sk)->hdev) > - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id); > + hdev = hci_hdev_from_sock(sk); > + if (!IS_ERR(hdev)) > + hdr->index = cpu_to_le16(hdev->id); Same deal. The 'id' is valid even if it's unregistered. So either it should have generated an error earlier, or we just shouldn't care. The fact that the old code used to do HCI_DEV_NONE seems to be more about the fact that the ID no longer existed. I think that if you want to monitor a socket with a stale hdev, that's likely pointless but valid. So I think this should just keep using the hdev->id, even for a HCI_UNREGISTER case. That said, the *normal* case seems to be from the socket ioctl code, so it's either explicitly not yet registered, or it just got registered with a hdev, so I don't think it even matters. It looks like the only case that HCI_UNREGISTER case would happen is likely the magical replay case. Which - again - I think would actually prefer the now still existing hdev channel id. > @@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) > > hdr = skb_push(skb, HCI_MON_HDR_SIZE); > hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE); > - if (hci_pi(sk)->hdev) > - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id); > + hdev = hci_hdev_from_sock(sk); > + if (!IS_ERR(hdev)) > + hdr->index = cpu_to_le16(hdev->id); > else > hdr->index = cpu_to_le16(HCI_DEV_NONE); > hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE); I think the monitor_ctrl close case is exactly the same case as the open case above. But the others look good to me. So I *think* that the cases you actually want to catch are just the ioctl/recvmsg/sendmsg ones. Not the special "monitor the hdev" ones. That said, if you have some test-case that argues differently, then hard data is obviously more valid than my blathering above. Linus
On 2021/08/03 2:36, Linus Torvalds wrote: > On Mon, Aug 2, 2021 at 8:19 AM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> @@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb) >> sk_for_each(sk, &hci_sk_list.head) { >> struct sk_buff *nskb; >> >> - if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev) >> + if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev) >> continue; >> >> /* Don't send frame to the socket it came from */ > > I'm not convinced this is a good idea. Well, you are right. One caller (out of three callers) of hci_send_to_sock() is hci_si_event() from hci_sock_dev_event(event <= HCI_DEV_DOWN) path. This change for hci_si_event() path is wrong, for hci_si_event() calls hci_send_to_sock() with hdev == NULL while hci_hdev_from_sock() != NULL. if (sk->sk_state != BT_BOUND || 1) continue; makes whole hci_send_to_sock(NULL) no-op. However, > > Here we use hci_hdev_from_sock() outside the socket lock, so now that > HCI_UNREGISTER test is not very logical. > > And it's positively stupid to start with a hdev, then walk the socket > list to look up another hdev, and then check "was it the original > hdev, but not unregistered"? > > In other words - if you care about HCI_UNREGISTER state, it should be > *above* the whole loop. Not inside of it. > > So I think this test should remain just that > > hci_pi(sk)->hdev != hdev > > and if HCI_UNREGISTER is relevant - and I don't think it is - it > should have been done earlier. now that this patch is going to remove sk->sk_state = BT_OPEN; hci_pi(sk)->hdev = NULL; from the sk_for_each(sk, &hci_sk_list.head) loop in hci_sock_dev_event(HCI_DEV_UNREG), hci_sock_dev_event(event <= HCI_DEV_DOWN) after hci_sock_dev_event(HCI_DEV_UNREG) will fail to hit the "continue;" path. The other two callers of hci_send_to_sock() are hci_send_frame() and hci_rx_work(). These hdev are not NULL but are subjected to hci_sock_dev_event(HCI_DEV_UNREG) race. Prior to this patch, if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev) continue; was not hitting "continue;" path before hci_sock_dev_event(HCI_DEV_UNREG) and was hitting "continue;" path after hci_sock_dev_event(HCI_DEV_UNREG), and the same problem (i.e. failing to hit "continue;" path) exists. Do we want to block sending frames to RAW sockets which are using dead device? If yes, doing if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev || (hci_pi(sk)->hdev && hci_dev_test_flag(hci_pi(sk)->hdev, HCI_UNREGISTER))) continue; will block sending frames from hci_sock_dev_event(event <= HCI_DEV_DOWN) because hci_dev_set_flag(hdev, HCI_UNREGISTER) was already called by hci_unregister_dev(). If no, just keeping if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev) continue; is correct. > >> @@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk) >> >> hdr = skb_push(skb, HCI_MON_HDR_SIZE); >> hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN); >> - if (hci_pi(sk)->hdev) >> - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id); Prior to this patch, compiler might have generated code that reads hci_pi(sk)->hdev for two times, and hci_sock_dev_event(HCI_DEV_UNREG) might reset hci_pi(sk)->hdev to NULL between these two reads; leading to NULL pointer dereference. >> + hdev = hci_hdev_from_sock(sk); >> + if (!IS_ERR(hdev)) >> + hdr->index = cpu_to_le16(hdev->id); > > Same deal. The 'id' is valid even if it's unregistered. > > So either it should have generated an error earlier, or we just shouldn't care. Well, maybe hdev = READ_ONCE(hci_pi(sk)->hdev); if (!hdev) hdr->index = cpu_to_le16(hdev->id); is the better for create_monitor_ctrl_open() and create_monitor_ctrl_close(), for it seems that create_monitor_ctrl_close() should report the same id. However, create_monitor_ctrl_close() from hci_sock_bind() is always using HCI_DEV_NONE because it is reachable only if hci_pi(sk)->hdev == NULL. Only create_monitor_ctrl_close() from hci_sock_release() has possibility of being able to use hdev->id rather than HCI_DEV_NONE. And > > The fact that the old code used to do HCI_DEV_NONE seems to be more > about the fact that the ID no longer existed. I think that if you > want to monitor a socket with a stale hdev, that's likely pointless > but valid. > > So I think this should just keep using the hdev->id, even for a > HCI_UNREGISTER case. > > That said, the *normal* case seems to be from the socket ioctl code, > so it's either explicitly not yet registered, or it just got > registered with a hdev, so I don't think it even matters. It looks > like the only case that HCI_UNREGISTER case would happen is likely the > magical replay case. > > Which - again - I think would actually prefer the now still existing > hdev channel id. prior to this patch, hci_sock_dev_event(HCI_DEV_UNREG) might have suddenly reset hci_pi(sk)->hdev to NULL, and create_monitor_ctrl_close() was forced to choose HCI_DEV_NONE rather than the id create_monitor_ctrl_open() used. Therefore, I think that the userspace needs to be designed to tolerate erroneous HCI_DEV_NONE and take appropriate recovery action if HCI_DEV_NONE was reported. So, I can't judge whether changing to hdev = hci_hdev_from_sock(sk); if (!IS_ERR(hdev)) hdr->index = cpu_to_le16(hdev->id); makes things better or worse. > > >> @@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) >> >> hdr = skb_push(skb, HCI_MON_HDR_SIZE); >> hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE); >> - if (hci_pi(sk)->hdev) >> - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id); >> + hdev = hci_hdev_from_sock(sk); >> + if (!IS_ERR(hdev)) >> + hdr->index = cpu_to_le16(hdev->id); >> else >> hdr->index = cpu_to_le16(HCI_DEV_NONE); >> hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE); > > I think the monitor_ctrl close case is exactly the same case as the > open case above. > > But the others look good to me. > > So I *think* that the cases you actually want to catch are just the > ioctl/recvmsg/sendmsg ones. Not the special "monitor the hdev" ones. > > That said, if you have some test-case that argues differently, then > hard data is obviously more valid than my blathering above. The special "monitor the hdev" is always racy. But "monitor the hdev" change is too subtle to handle within this patch. Leaving "monitor the hdev" change to future patches might be the better. I want opinions from Bluetooth developers.
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..39c03a8b762f 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); @@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb) sk_for_each(sk, &hci_sk_list.head) { struct sk_buff *nskb; - if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev) + if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev) continue; /* Don't send frame to the socket it came from */ @@ -494,6 +505,7 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk) u16 format; u8 ver[3]; u32 flags; + struct hci_dev *hdev; /* No message needed when cookie is not present */ if (!hci_pi(sk)->cookie) @@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk) hdr = skb_push(skb, HCI_MON_HDR_SIZE); hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN); - if (hci_pi(sk)->hdev) - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id); + hdev = hci_hdev_from_sock(sk); + if (!IS_ERR(hdev)) + hdr->index = cpu_to_le16(hdev->id); else hdr->index = cpu_to_le16(HCI_DEV_NONE); hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE); @@ -549,6 +562,7 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) { struct hci_mon_hdr *hdr; struct sk_buff *skb; + struct hci_dev *hdev; /* No message needed when cookie is not present */ if (!hci_pi(sk)->cookie) @@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk) hdr = skb_push(skb, HCI_MON_HDR_SIZE); hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE); - if (hci_pi(sk)->hdev) - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id); + hdev = hci_hdev_from_sock(sk); + if (!IS_ERR(hdev)) + hdr->index = cpu_to_le16(hdev->id); else hdr->index = cpu_to_le16(HCI_DEV_NONE); hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE); @@ -759,19 +774,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 +939,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 +1112,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; @@ -1110,7 +1131,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, switch (haddr.hci_channel) { case HCI_CHANNEL_RAW: - if (hci_pi(sk)->hdev) { + if (hdev) { err = -EALREADY; goto done; } @@ -1157,7 +1178,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, break; case HCI_CHANNEL_USER: - if (hci_pi(sk)->hdev) { + if (hdev) { err = -EALREADY; goto done; } @@ -1379,9 +1400,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 +1764,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. 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> Tested-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com> Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object") --- 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 | 67 +++++++++++++++++++++----------- net/bluetooth/hci_sysfs.c | 3 ++ 4 files changed, 56 insertions(+), 31 deletions(-)