2018-03-21 19:22:49

by Colin King

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

From: Colin Ian King <[email protected]>

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 <[email protected]>
---
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

2018-03-21 20:05:01

by Joe Perches

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

On Wed, 2018-03-21 at 19:19 +0000, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);

2018-03-26 15:38:08

by Ajay Singh

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

Thanks for submitting the patch.

On Wed, 21 Mar 2018 13:03:18 -0700
Joe Perches <[email protected]> wrote:

> On Wed, 2018-03-21 at 19:19 +0000, Colin King wrote:
> > From: Colin Ian King <[email protected]>
> >
> > 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

2018-03-26 16:17:09

by Colin King

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

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 <[email protected]> wrote:
>
>> On Wed, 2018-03-21 at 19:19 +0000, Colin King wrote:
>>> From: Colin Ian King <[email protected]>
>>>
>>> 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
>