Re: [PATCH v2] Bluetooth: Fix UAF for hci_chan

["Ziyang Xuan (William)" <william.xuanziyang@huawei.com>]

> Hi Pauli,
> 
> On Tue, Oct 10, 2023 at 11:32 AM Pauli Virtanen <pav@iki.fi> wrote:
>>
>> 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
>>
>> ?
> 
> Either way Im not sure what the IDA logic has to with it, that said I
> think using ida function is actually a much better solution then the
> lookup one so I would be happy to apply it if someone split the
> changes related to it and send a patch.

I will send a patch just with handle ida modification. The remaining UAF issues
continue to be tracked.

> 
>> 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?
>>