On 2021/07/22 3:17, Luiz Augusto von Dentz wrote: > I think it would have been cleaner if we have dedicated functions for > each command like I did in my patch: > > https://patchwork.kernel.org/project/bluetooth/patch/20210717000731.3836303-1-luiz.dentz@xxxxxxxxx/ But your patch was proven to be unsafe. There is a use-after-unregister race window which would require at least 1000 lines of modification and a lot of careful review if we try to manage without my patch. Such all-in-one-step change is too much for "sleep in atomic context" regression fix which is preventing syzbot from testing Bluetooth module and is preventing Linux distributors from fixing CVE-2021-3573. As far as I can see, it is lock_sock() (not bh_lock_sock_nested() in your patch) that is needed for protecting sk->sk_err = EPIPE; sk->sk_state = BT_OPEN; sk->sk_state_change(sk); in hci_sock_dev_event(HCI_DEV_UNREG) from concurrent modification lock_sock(sk); if (sk->sk_state == BT_BOUND) { err = -EALREADY; goto done; } (...snipped...) - hci_pi(sk)->hdev = hdev; + if (hdev) { + hci_pi(sk)->dev = hdev->id; + hci_dev_put(hdev); + } (...snipped...) /* Race window is here. */ (...snipped...) sk->sk_state = BT_BOUND; done: release_sock(sk); in hci_sock_bind(). > > That way it is simpler to protect the likes of > copy_from_user/copy_to_user, etc, even if we have to some checks > duplicated on each function we can have a helper function to checks > the flags, etc. My patch calls copy_from_user()/copy_to_user() without lock_sock() which works nicely with "[PATCH v3] Bluetooth: call lock_sock() outside of spinlock section". I'd like to backport "[PATCH v2] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()" together.
