wifi: plfxlc: fix potential NULL pointer dereference in plfxlc_usb_wreq_async()

[PATCH] wifi: plfxlc: fix potential NULL pointer dereference in plfxlc_usb_wreq_async()

Posted by Zheng Wang 2 years, 7 months ago

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);
+		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);
 
-- 
2.25.1

Re: [PATCH] wifi: plfxlc: fix potential NULL pointer dereference in plfxlc_usb_wreq_async()

Posted by Christophe JAILLET 2 years, 7 months ago

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);
>

Re:Re: [PATCH] wifi: plfxlc: fix potential NULL pointer dereference in plfxlc_usb_wreq_async()

Posted by 王征 2 years, 7 months ago
















At 2023-02-03 13:25:45, "Christophe JAILLET" <christophe.jaillet@wanadoo.fr> wrote:
>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?


Hello CJ,


Thanks for pointing that out. Kfree is unnecessary in such case, we can just return.


Regards,
Zheng Wang

>
>> +		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);
>>