Re: [PATCH] Bluetooth: fix UAF read of ->accept_q in bt_accept_poll()

[Jann Horn]

On Tue, May 5, 2026 at 5:06 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> On Mon, May 4, 2026 at 11:11 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
> >
> > Use lock_sock() to guard against bt_accept_poll() racing with concurrent
> > close(accept()), which can lead to UAF:
> >
> > task 1           task 2
> > ======           ======
> >                  __x64_sys_poll
> >                    __se_sys_poll
> >                      __do_sys_poll
> >                        do_sys_poll
> >                          do_poll
> >                            do_pollfd
> >                              vfs_poll
> >                                sock_poll
> >                                  bt_sock_poll
> >                                    bt_accept_poll
> >                                      [read ->accept_q next pointer]
> > __x64_sys_accept
> >   __se_sys_accept
> >     __do_sys_accept
> >       __sys_accept4
> >         __sys_accept4_file
> >           do_accept
> >             l2cap_sock_accept
> >               bt_accept_dequeue
> >                 bt_accept_unlink
> >                   [removes new socket from ->accept_q]
> > __x64_sys_close
> >   __se_sys_close
> >     __do_sys_close
> >       fput_close_sync
> >         __fput
> >           sock_close
> >             __sock_release
> >               l2cap_sock_release
> >                 l2cap_sock_kill
> >                   sock_put
> >                     sk_free
> >                       __sk_free
> >                         sk_destruct
> >                           __sk_destruct
> >                             [frees new socket]
> >                                      [UAF read of ->sk_state]
> >
> > This UAF only leads to incorrect reads, it does not corrupt memory; it is a
> > fairly tight race window; I believe every race attempt requires an
> > incoming bluetooth connection; and the leaked data is limited.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > ---
> >  net/bluetooth/af_bluetooth.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> > index 33d053d63407..d24897167838 100644
> > --- a/net/bluetooth/af_bluetooth.c
> > +++ b/net/bluetooth/af_bluetooth.c
> > @@ -521,13 +521,17 @@ static inline __poll_t bt_accept_poll(struct sock *parent)
> >         struct bt_sock *s, *n;
> >         struct sock *sk;
> >
> > +       lock_sock(parent);
> >         list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
> >                 sk = (struct sock *)s;
> >                 if (sk->sk_state == BT_CONNECTED ||
> >                     (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags) &&
> > -                    sk->sk_state == BT_CONNECT2))
> > +                    sk->sk_state == BT_CONNECT2)) {
> > +                       release_sock(parent);
> >                         return EPOLLIN | EPOLLRDNORM;
> > +               }
> >         }
> > +       release_sock(parent);
>
> There is the following comments though:
>
> https://sashiko.dev/#/patchset/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd%40google.com

Regarding the LLM output on whether lock_sock(parent) is enough: The
locking I'm adding here is the same as what bt_accept_dequeue() uses
for protection; if event handling can also remove accept_q elements
without holding appropriate locks, I think that is a separate (and
bigger) bug.

I see I've just been CC'ed on
<https://lore.kernel.org/all/20260506114338.2873496-1-n05ec@xxxxxxxxxx/>,
which seems to be a broader fix; if you want to go with that patch,
this one is superfluous.

> I'm not really sure if likes for the poll are supposed to be done
> lockless, if they are, we cannot use lock_sock here and will likely
> need to rework accept_q so it doesn't contain deferred sks, as those
> shouldn't be considered ready for acceptance.

I don't see why that would be a problem;
Documentation/filesystems/vfs.rst says nothing about wanting lockless
operation, and if you look around at other poll handlers, you'll see
several ->poll() handlers that take sleeping locks:

 - dma_buf_poll() calls dma_resv_lock(), which locks a W/W mutex
 - vb2_fop_poll() sometimes calls mutex_lock_interruptible()
 - virtio_rpmsg_poll() calls mutex_lock()

My understanding is that is is preferable, but not required, for
->poll() handlers to be fast if they're used by high-performance
userspace code, since event loops might hit ->poll() handlers fairly
often (especially if userspace uses an API like poll() or select(); I
think with epoll you only get one or two ->poll() callbacks once the
file descriptor actually becomes ready); I think this probably isn't
really an issue for bluetooth listening sockets.