linux-kernel - Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM9PR04MB860329622408ED0DC4D1CCC6E7D4A@AM9PR04MB8603.eurprd04.prod.outlook.com>
Date:   Thu, 19 Oct 2023 09:41:35 +0000
From:   Neeraj sanjay kale <neeraj.sanjaykale@....com>
To:     Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>
CC:     "luiz.dentz@...il.com" <luiz.dentz@...il.com>,
        Sherry Sun <sherry.sun@....com>,
        "johan.hedberg@...il.com" <johan.hedberg@...il.com>,
        Amitkumar Karwar <amitkumar.karwar@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "marcel@...tmann.org" <marcel@...tmann.org>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

Hi Marcel

> > > From: Marcel Ziswiler <marcel.ziswiler@...adex.com>
> > >
> > > Unfortunately, btnxpuart_close() may trigger a BUG: scheduling while
> atomic.
> > > Fix this by properly purging the transmit queue and freeing the receive
> skb.
> > >
> > > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP
> > > Bluetooth chipsets")
> > >
> > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@...adex.com>
> > > ---
> > >
> > >  drivers/bluetooth/btnxpuart.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/bluetooth/btnxpuart.c
> > > b/drivers/bluetooth/btnxpuart.c index b7e66b7ac570..9cb7529eef09
> > > 100644
> > > --- a/drivers/bluetooth/btnxpuart.c
> > > +++ b/drivers/bluetooth/btnxpuart.c
> > > @@ -1234,6 +1234,9 @@ static int btnxpuart_close(struct hci_dev
> > > *hdev)
> > >
> > >         ps_wakeup(nxpdev);
> > >         serdev_device_close(nxpdev->serdev);
> > > +       skb_queue_purge(&nxpdev->txq);
> > > +       kfree_skb(nxpdev->rx_skb);
> > > +       nxpdev->rx_skb = NULL;
> > >         clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state);
> > >         return 0;
> > >  }
> > This is already done in btnxpuart_flush(), which is called by
> > hci_dev_close_sync(), before it calls btnxpuart_close().
> 
> Yes, I was also wondering about that.
> 
> > Is btnxpuart_flush() not called during your testing?
> 
> Yes, I even added some more tracing to confirm this. However, without my
> fix (which BTW was inspired by looking at the former hci_mrvl.c driver) this
> bug is really occuring. Just keep loading/un-loading the kernel module a few
> times any you may hit it.
> 
Our QA team has been running load/unload tests for quite some time now, and
such an issue was never reported.
I am not sure why you do not see the btnxpuart_flush () been called, but I tested this patch
on my setup, where both, btnxpuart_flush() and btnxpuart_close() are called, and I
don’t see any issue due to kfree_skb and txq purge been done twice.

This looks ok to me.

Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>