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: <YI+s2Hms/56Pvatu@hovoldconsulting.com>
Date:   Mon, 3 May 2021 09:57:12 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com, luiz.dentz@...il.com,
        linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rocky Liao <rjliao@...eaurora.org>,
        Matthias Kaehlcke <mka@...omium.org>
Subject: Re: [PATCH] bluetooth: fix potential gfp

On Sat, May 01, 2021 at 06:04:45PM +0300, Pavel Skripkin wrote:
> In qca_power_shutdown() qcadev local variable is
> initialized by hu->serdev.dev private data, but
> hu->serdev can be NULL and there is a check for it.
> 
> Since, qcadev is not used before
> 
> 	if (!hu->serdev)
> 		return;
> 
> we can move its initialization after this "if" to
> prevent gfp.

Good catch. The commit message needs to be improved however.

First, what's a "gfp"? Did you mean GPF?

Second, I'd expect you to try to point to the commit that introduced
this issue (e.g. using a Fixes tag) and CC the person responsible. This
appears to be commit 5559904ccc08 ("Bluetooth: hci_qca: Add QCA Rome
power off support to the qca_power_shutdown()") but you should verify
that.

Third, this looks like it could be triggered by user space so you should
CC stable too so that the fix is backported.

Fourth, your commit summary (Subject) is missing the driver component
(i.e. "hci_qca").

> Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
> ---
>  drivers/bluetooth/hci_qca.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index de36af63e182..9589ef6c0c26 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1820,8 +1820,6 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	unsigned long flags;
>  	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>  
> -	qcadev = serdev_device_get_drvdata(hu->serdev);
> -
>  	/* From this point we go into power off state. But serial port is
>  	 * still open, stop queueing the IBS data and flush all the buffered
>  	 * data in skb's.
> @@ -1837,6 +1835,8 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	if (!hu->serdev)
>  		return;
>  
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +
>  	if (qca_is_wcn399x(soc_type)) {
>  		host_set_baudrate(hu, 2400);
>  		qca_send_power_pulse(hu, false);

Johan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ