Re: [PATCH v2] Bluetooth: SCO: give the socket its own sco_conn reference

[Aldo Ariel Panzardo <qwe.aldo@gmail.com>]

Hi Pauli,

Thanks a lot for the detailed review and for sketching the hci_conn
ownership rule -- v3 incorporates it (you're on the patch as Suggested-by):
sco_conn_add() now consumes one hci_conn reference and the sco_conn owns it
for its whole lifetime, sco_connect() no longer drops hcon on its error
paths, sco_connect_cfm() takes a reference before sco_conn_add() (and drops
it again if the allocation fails), and the hci_conn_hold() in
sco_conn_ready() is gone. That removes the double hci_conn_drop() you
spotted on the error paths.

One correction to v2: I had also dropped the

	if (conn->sk)
		sco_pi(conn->sk)->conn = NULL;

clearing in sco_conn_free() as "unreachable". v3 keeps it. Testing the
close()-vs-Disconnection-Complete race under KASAN showed that removing it
reintroduces a use-after-free on the sco_sock_release() path, so it stays.

While validating this I built a /dev/vhci reproducer that races close() of
an SCO socket against an injected Disconnection Complete over many
iterations, on v7.2-rc4 with KASAN. It reproduces the sco_conn_del()
over-put reliably on an unmodified tree (a few thousand iterations), and
with this patch that over-put no longer fires.

I want to be upfront that it does not leave the reproducer fully clean,
though. With the over-put fixed, the same race still occasionally trips a
separate use-after-free in the SCODATA receive path, which this patch does
not touch:

    BUG: KASAN: slab-use-after-free in sco_conn_hold_unless_zero+0xbe/0x160
    Write of size 4 by task kworker/u17:0
    Workqueue: hci0 hci_rx_work
    Call Trace:
     sco_conn_hold_unless_zero+0xbe/0x160
     sco_recv_scodata+0x13f/0x490
     hci_rx_work+0x3af/0x730

sco_recv_scodata() takes conn from hcon->sco_data and calls
sco_conn_hold_unless_zero() on it, but kref_get_unless_zero() only guards
against a zero refcount, not against the sco_conn already having been
freed and its memory reclaimed -- the read of the refcount itself is the
UAF. It looks like a pre-existing race around the hcon->sco_data weak
pointer rather than something introduced here, but I haven't yet isolated
it on an otherwise-unmodified tree, so I didn't want to fold a fix into
this patch blind.

Happy to dig into that one next (it feels like the sco_conn lifetime wants
a firmer owner for hcon->sco_data), either as a follow-up or however you'd
prefer to see it handled. Let me know if you'd rather I respin this
together with that.

Thanks,
Aldo