linux-kernel - Re: [PATCH v1 1/3] Bluetooth: btnxpuart: Fix Null pointer dereference in btnxpuart

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z9i1b9hzZzwJnIYh@atmark-techno.com>
Date: Tue, 18 Mar 2025 08:51:11 +0900
From: Dominique MARTINET <dominique.martinet@...ark-techno.com>
To: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
Cc: marcel@...tmann.org, luiz.dentz@...il.com,
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
	amitkumar.karwar@....com, rohit.fule@....com, sherry.sun@....com,
	ziniu.wang_1@....com, haibo.chen@....com, LnxRevLi@....com,
	guillaume.legoupil@....com, salim.chebbo@....com
Subject: Re: [PATCH v1 1/3] Bluetooth: btnxpuart: Fix Null pointer
 dereference in btnxpuart_flush()

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