Hi,
ti, 2023-10-10 kello 20:49 +0800, Ziyang Xuan (William)
kirjoitti:[clip]
> > >
> > > - if (atomic_dec_and_test(&conn->refcnt)) {
> > > + if (atomic_dec_and_test(&conn->refcnt) &&
> > > + !test_bit(HCI_CONN_DISC, &conn->flags)) {
> > > unsigned long timeo;
> >
> > hci_abort_conn already checks if conn->abort_reason was already set, to
> > avoid queuing abort for the same conn again. Does this flag not try to
> > do the same thing?
>
> That is not enough. hci_conn occur UAF in hci_proto_disconn_ind() before enter
> hci_abort_conn().
Thanks, this was not clear to me from the patch.
So the problem is that the cancel_delayed_work_sync(&conn->disc_work)
in hci_conn_del doesn't help in a few cases:
1. [task A] hci_conn_del(conn) entered
2. [A] cancel_delayed_work_sync(conn->disc_work)
3. [B] concurrent hci_conn_drop(conn) queues disc_work again
4. [A] hci_conn_del finishes
5. UAF when disc_work runs
or without needing concurrency
1. hci_conn_del(conn) entered and finishes
2. hci_conn_drop(conn); hci_conn_put(conn); as done in several places
?
The hci_conn_del here is not necessarily from hci_abort_conn. Should
the atomic flag be set in hci_conn_del before
cancel_delayed_work_sync(&conn->disc_work) to catch possible other
cases?
> > There are potential races in hci_sync vs. handle reuse since the queued
> > abort is not canceled if the conn is deleted by something else. But now
> > I'm not sure syzbot hits this race.
>
> Sorry, can you give a specific scenario. I can't understand exactly what you mean.
As noted in the other patch:
1. hci_conn_abort(conn)
2. hci_cmd_sync_queue(hdev,abort_conn_sync,UINT_PTR(conn->handle))
3. something else (e.g. HCI event) deletes conn
4. something else (e.g. HCI event) creates conn2, with same handle
value
5. Queued abort_conn_sync executes. It needs to be delayed enough,
but doesn't need to be concurrent.
6. abort_conn_sync uses queued handle value, finds conn2 (not conn)
7. hci_abort_conn_sync(conn2, conn2->abort_reason)
8. Calling hci_abort_conn_sync with reason == 0 causes UAF
The UAF is because reason==0 is passed to l2cap_disconn_ind which does
not signal disconnection to the L2CAP layer, hci_abort_conn_sync
deletes conn immediately after that, and later on L2CAP tries to access
stale conn pointer.
> > >
> > > switch (conn->type) {
> > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > > index 974631e652c1..f87281b4386f 100644
> > > --- a/net/bluetooth/hci_conn.c
> > > +++ b/net/bluetooth/hci_conn.c
> > > @@ -153,6 +153,9 @@ static void hci_conn_cleanup(struct hci_conn *conn)
> > >
> > >
> > >
> > >
> > > hci_conn_hash_del(hdev, conn);
> > >
> > >
> > >
> > >
> > > + if (HCI_CONN_HANDLE_UNSET(conn->handle))
> > > + ida_free(&hdev->unset_handle_ida, conn->handle);
> > > +
> > > if (conn->cleanup)
> > > conn->cleanup(conn);
> > >
> > >
> > >
> > >
> > > @@ -929,29 +932,17 @@ static void cis_cleanup(struct hci_conn *conn)
> > > hci_le_remove_cig(hdev, conn->iso_qos.ucast.cig);
> > > }
> > >
> > >
> > >
> > >
> > > -static u16 hci_conn_hash_alloc_unset(struct hci_dev *hdev)
> > > +static int hci_conn_hash_alloc_unset(struct hci_dev *hdev)
> > > {
> > > - struct hci_conn_hash *h = &hdev->conn_hash;
> > > - struct hci_conn *c;
> > > - u16 handle = HCI_CONN_HANDLE_MAX + 1;
> > > -
> > > - rcu_read_lock();
> > > -
> > > - list_for_each_entry_rcu(c, &h->list, list) {
> > > - /* Find the first unused handle */
> > > - if (handle == 0xffff || c->handle != handle)
> > > - break;
> > > - handle++;
> > > - }
> > > - rcu_read_unlock();
> >
> > The original hci_conn_hash_alloc_unset seems to have wrong logic. It'll
> > e.g. always return HCI_CONN_HANDLE_MAX+1 except if the first conn in
> > conn_hash (which is not in particular order) has that handle...
> >
> > The code paths adding/deleting conns or entering here / setting handles
> > should be holding hdev->lock, so probably no concurrency issue in it.
> >
> > Is syzbot happy with just this handle allocation fixed?
>
> Just fix handle, hci_conn occur UAF in hci_proto_disconn_ind() within hci_conn_timeout() process.
>
> >
> > > -
> > > - return handle;
> > > + return ida_alloc_range(&hdev->unset_handle_ida, HCI_CONN_HANDLE_MAX + 1,
> > > + U16_MAX, GFP_ATOMIC);
> > > }
> > >
> > >
> > >
> > >
> >
--
Pauli Virtanen
