[PATCH] staging: wilc1000: check for kmalloc allocation failures

[PATCH] staging: wilc1000: check for kmalloc allocation failures

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

There are three kmalloc allocations that are not null checked which
potentially could lead to null pointer dereference issues. Fix this
by adding null pointer return checks.

Detected by CoverityScan, CID#1466025-27 ("Dereference null return")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/staging/wilc1000/host_interface.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 5082ede720f0..9b9b86654958 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -944,6 +944,10 @@ static s32 handle_connect(struct wilc_vif *vif,
 
 	if (conn_attr->bssid) {
 		hif_drv->usr_conn_req.bssid = kmalloc(6, GFP_KERNEL);
+		if (!hif_drv->usr_conn_req.bssid) {
+			result = -ENOMEM;
+			goto error;
+		}
 		memcpy(hif_drv->usr_conn_req.bssid, conn_attr->bssid, 6);
 	}
 
@@ -951,6 +955,10 @@ static s32 handle_connect(struct wilc_vif *vif,
 	if (conn_attr->ssid) {
 		hif_drv->usr_conn_req.ssid = kmalloc(conn_attr->ssid_len + 1,
 						     GFP_KERNEL);
+		if (!hif_drv->usr_conn_req.ssid) {
+			result = -ENOMEM;
+			goto error;
+		}
 		memcpy(hif_drv->usr_conn_req.ssid,
 		       conn_attr->ssid,
 		       conn_attr->ssid_len);
@@ -961,6 +969,10 @@ static s32 handle_connect(struct wilc_vif *vif,
 	if (conn_attr->ies) {
 		hif_drv->usr_conn_req.ies = kmalloc(conn_attr->ies_len,
 						    GFP_KERNEL);
+		if (!hif_drv->usr_conn_req.ies) {
+			result = -ENOMEM;
+			goto error;
+		}
 		memcpy(hif_drv->usr_conn_req.ies,
 		       conn_attr->ies,
 		       conn_attr->ies_len);
-- 
2.15.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures

[Joe Perches]

On Wed, 2018-03-21 at 19:19 +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> There are three kmalloc allocations that are not null checked which
> potentially could lead to null pointer dereference issues. Fix this
> by adding null pointer return checks.

looks like all of these should be kmemdup or kstrdup

> Detected by CoverityScan, CID#1466025-27 ("Dereference null return")
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 5082ede720f0..9b9b86654958 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -944,6 +944,10 @@ static s32 handle_connect(struct wilc_vif *vif,
>  
>  	if (conn_attr->bssid) {
>  		hif_drv->usr_conn_req.bssid = kmalloc(6, GFP_KERNEL);
> +		if (!hif_drv->usr_conn_req.bssid) {
> +			result = -ENOMEM;
> +			goto error;
> +		}
>  		memcpy(hif_drv->usr_conn_req.bssid, conn_attr->bssid, 6);
>  	}
>  
> @@ -951,6 +955,10 @@ static s32 handle_connect(struct wilc_vif *vif,
>  	if (conn_attr->ssid) {
>  		hif_drv->usr_conn_req.ssid = kmalloc(conn_attr->ssid_len + 1,
>  						     GFP_KERNEL);
> +		if (!hif_drv->usr_conn_req.ssid) {
> +			result = -ENOMEM;
> +			goto error;
> +		}
>  		memcpy(hif_drv->usr_conn_req.ssid,
>  		       conn_attr->ssid,
>  		       conn_attr->ssid_len);
> @@ -961,6 +969,10 @@ static s32 handle_connect(struct wilc_vif *vif,
>  	if (conn_attr->ies) {
>  		hif_drv->usr_conn_req.ies = kmalloc(conn_attr->ies_len,
>  						    GFP_KERNEL);
> +		if (!hif_drv->usr_conn_req.ies) {
> +			result = -ENOMEM;
> +			goto error;
> +		}
>  		memcpy(hif_drv->usr_conn_req.ies,
>  		       conn_attr->ies,
>  		       conn_attr->ies_len);

Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures

[Ajay Singh]

Thanks for submitting the patch.

On Wed, 21 Mar 2018 13:03:18 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2018-03-21 at 19:19 +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > There are three kmalloc allocations that are not null checked which
> > potentially could lead to null pointer dereference issues. Fix this
> > by adding null pointer return checks.
> 
> looks like all of these should be kmemdup or kstrdup
> 
> >  
> > @@ -951,6 +955,10 @@ static s32 handle_connect(struct wilc_vif *vif,
> >  	if (conn_attr->ssid) {
> >  		hif_drv->usr_conn_req.ssid = kmalloc(conn_attr->ssid_len + 1,
> >  						     GFP_KERNEL);
> > +		if (!hif_drv->usr_conn_req.ssid) {
> > +			result = -ENOMEM;
> > +			goto error;
> > +		}
> >  		memcpy(hif_drv->usr_conn_req.ssid,
> >  		       conn_attr->ssid,
> >  		       conn_attr->ssid_len);

With this changes the Coverity reported warning is handled correctly.

For further improvement to the patch, as Joe Perches suggested, its better
to make use of kmemdup instead of kmalloc & memcpy. As kstrdup requires the
source string to be NULL terminated('\0') and conn_attr->ssid might not  
contains the '\0' terminated string. So kmemdup with length of 
'conn_attr->ssid_len' can be used instead.

Please include the changes by using kmemdup() for all kmalloc/memcpy in
this patch.



Regards,
Ajay

Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures

[Colin Ian King]

On 26/03/18 16:35, Ajay Singh wrote:
> Thanks for submitting the patch.
> 
> On Wed, 21 Mar 2018 13:03:18 -0700
> Joe Perches <joe@perches.com> wrote:
> 
>> On Wed, 2018-03-21 at 19:19 +0000, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> There are three kmalloc allocations that are not null checked which
>>> potentially could lead to null pointer dereference issues. Fix this
>>> by adding null pointer return checks.
>>
>> looks like all of these should be kmemdup or kstrdup
>>
>>>  
>>> @@ -951,6 +955,10 @@ static s32 handle_connect(struct wilc_vif *vif,
>>>  	if (conn_attr->ssid) {
>>>  		hif_drv->usr_conn_req.ssid = kmalloc(conn_attr->ssid_len + 1,
>>>  						     GFP_KERNEL);
>>> +		if (!hif_drv->usr_conn_req.ssid) {
>>> +			result = -ENOMEM;
>>> +			goto error;
>>> +		}
>>>  		memcpy(hif_drv->usr_conn_req.ssid,
>>>  		       conn_attr->ssid,
>>>  		       conn_attr->ssid_len);
> 
> With this changes the Coverity reported warning is handled correctly.
> 
> For further improvement to the patch, as Joe Perches suggested, its better
> to make use of kmemdup instead of kmalloc & memcpy. As kstrdup requires the
> source string to be NULL terminated('\0') and conn_attr->ssid might not  
> contains the '\0' terminated string. So kmemdup with length of 
> 'conn_attr->ssid_len' can be used instead.
> 
> Please include the changes by using kmemdup() for all kmalloc/memcpy in
> this patch.

The original has been included into Greg's staging repo, so I'll send a
send patch that addresses the kmemdup.

Colin
> 
> 
> 
> Regards,
> Ajay
>