Message-ID: <dd1c45ad-7af2-8df1-a3ab-0db99dd25934@wanadoo.fr>
Date:   Fri, 3 Feb 2023 07:25:45 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] wifi: plfxlc: fix potential NULL pointer dereference in
 plfxlc_usb_wreq_async()
To:     Zheng Wang <zyytlz.wz@163.com>, srini.raju@purelifi.com
Cc:     kvalo@kernel.org, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230203041644.581649-1-zyytlz.wz@163.com>
Content-Language: fr
From:   Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In-Reply-To: <20230203041644.581649-1-zyytlz.wz@163.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le 03/02/2023 à 05:16, Zheng Wang a écrit :
> Although the usb_alloc_urb uses GFP_ATOMIC, tring to make sure the memory
>   allocated not to be NULL. But in some low-memory situation, it's still
>   possible to return NULL. It'll pass urb as argument in
>   usb_fill_bulk_urb, which will finally lead to a NULL pointer dereference.
> 
> Fix it by adding additional check.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger.
> 
> Fixes: 68d57a07bfe5 ("wireless: add plfxlc driver for pureLiFi X, XL, XC devices")
> 
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> ---
>   drivers/net/wireless/purelifi/plfxlc/usb.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/wireless/purelifi/plfxlc/usb.c b/drivers/net/wireless/purelifi/plfxlc/usb.c
> index 76d0a778636a..ac149aa64908 100644
> --- a/drivers/net/wireless/purelifi/plfxlc/usb.c
> +++ b/drivers/net/wireless/purelifi/plfxlc/usb.c
> @@ -496,10 +496,17 @@ int plfxlc_usb_wreq_async(struct plfxlc_usb *usb, const u8 *buffer,
>   	struct urb *urb = usb_alloc_urb(0, GFP_ATOMIC);
>   	int r;
>   
> +	if (!urb) {
> +		r = -ENOMEM;
> +		kfree(urb);

Hi,
why kfree() in such a case?

CJ

> +		goto out;
> +	}
>   	usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, EP_DATA_OUT),
>   			  (void *)buffer, buffer_len, complete_fn, context);
>   
>   	r = usb_submit_urb(urb, GFP_ATOMIC);
> +
> +out:
>   	if (r)
>   		dev_err(&udev->dev, "Async write submit failed (%d)\n", r);
>