Re: [PATCH v1 1/3] Bluetooth: btnxpuart: Fix Null pointer dereference in btnxpuart_flush()

[Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>]

Hello Dominique,

> 
> Hi,
> 
> Neeraj Sanjay Kale wrote on Wed, May 15, 2024 at 12:36:55PM +0530:
> > @@ -1269,8 +1271,10 @@ static int btnxpuart_flush(struct hci_dev
> > *hdev)
> >
> >       cancel_work_sync(&nxpdev->tx_work);
> >
> > -     kfree_skb(nxpdev->rx_skb);
> > -     nxpdev->rx_skb = NULL;
> > +     if (!IS_ERR_OR_NULL(nxpdev->rx_skb)) {
> > +             kfree_skb(nxpdev->rx_skb);
> > +             nxpdev->rx_skb = NULL;
> > +     }
> 
> This is an old patch but I hit that on our slightly old tree and was wondering
> about this patch: why does flush() have to free rx at all?
> 
> I think this either needs a lock or (preferably) just remove this free:
> - This is inherently racy with btnxpuart_receive_buf() which is run in another
> workqueue with no lock involved as far as I understand, so this is not just
> about errors but you could also free something in a weird place here.
> As far as I understand, even if we don't do anything, the rx path will free the
> reply if no matching request was found.
> - looking at other drivers, the hdev->flush() call never does anything about rx
> and seems to only be about rx (ah, checking again as of master
> drivers/bluetooth/btmtkuart.c seems to have this same problem as of before
> this patch e.g. they're not checking for errors either... This probably needs
> something akin to this patch or removal as well. All other drivers have flush
> seem to be mostly about tx, but I do see some cancel work for rx as well so
> I'm a bit unclear as to what is expected of flush())

Well this comes from an example driver which Marcel had proposed for reference to create a standalone BT driver based on serdev.
https://lore.kernel.org/lkml/2991d8c04ce54f49aa292e9ac1968b7cb3b6a383.1530004712.git.sean.wang@mediatek.com/

When NXP (and probably Mediatek too) patches for hci_uart were rejected, standalone BTUART drivers were implemented based on this reference driver.

Later Francesco Dolcini added the rx_skb free logic in btnxpuart_close. To avoid breaking any functionality, we let rx_skb free logic to be kept in both places, close() and flush()
https://lore.kernel.org/all/20240304181421.14777-1-francesco@dolcini.it/T/

Based on the description you shared, I think it is safe to remove kfree_skb(rx_skb) from the flush() functions and keep it only in close() functions.

Thanks,
Neeraj