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

[Dominique MARTINET <dominique.martinet@atmark-techno.com>]

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())

Thank you,
-- 
Dominique