Hi Tetsuo, On Wed, Jul 21, 2021 at 4:42 PM Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote: > > 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. Im not saying you should adopt my solution, the locking etc stay the same as in this set but each command should have a helper function to make it clearer that way we don't have to re-evaluate the command over and over. > 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. Yep, Im not asking you to change any of that. -- Luiz Augusto von Dentz
