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:
 <AS4PR04MB969287FECB099EB02DD45EF2E7D82@AS4PR04MB9692.eurprd04.prod.outlook.com>
Date: Thu, 20 Mar 2025 10:20:24 +0000
From: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
To: Dominique MARTINET <dominique.martinet@...ark-techno.com>
CC: "marcel@...tmann.org" <marcel@...tmann.org>, "luiz.dentz@...il.com"
	<luiz.dentz@...il.com>, "linux-bluetooth@...r.kernel.org"
	<linux-bluetooth@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Amitkumar Karwar <amitkumar.karwar@....com>,
	Rohit Fule <rohit.fule@....com>, Sherry Sun <sherry.sun@....com>, Luke Wang
	<ziniu.wang_1@....com>, Bough Chen <haibo.chen@....com>, LnxRevLi
	<LnxRevLi@....com>, Guillaume Legoupil <guillaume.legoupil@....com>, Salim
 Chebbo <salim.chebbo@....com>
Subject: Re: [PATCH v1 1/3] Bluetooth: btnxpuart: Fix Null pointer dereference
 in btnxpuart_flush()

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