* [PATCH] staging: wilc1000: check for kmalloc allocation failures
@ 2018-03-21 19:19 ` Colin King
0 siblings, 0 replies; 9+ messages in thread
From: Colin King @ 2018-03-21 19:19 UTC (permalink / raw)
To: Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel
Cc: kernel-janitors, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
2018-03-21 19:19 ` Colin King
@ 2018-03-21 20:03 ` Joe Perches
-1 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2018-03-21 20:03 UTC (permalink / raw)
To: Colin King, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel
Cc: kernel-janitors, linux-kernel
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);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
@ 2018-03-21 20:03 ` Joe Perches
0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2018-03-21 20:03 UTC (permalink / raw)
To: Colin King, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel
Cc: kernel-janitors, linux-kernel
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);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
2018-03-21 20:03 ` Joe Perches
@ 2018-03-26 15:47 ` Ajay Singh
-1 siblings, 0 replies; 9+ messages in thread
From: Ajay Singh @ 2018-03-26 15:35 UTC (permalink / raw)
To: Colin King
Cc: Joe Perches, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
@ 2018-03-26 15:47 ` Ajay Singh
0 siblings, 0 replies; 9+ messages in thread
From: Ajay Singh @ 2018-03-26 15:47 UTC (permalink / raw)
To: Colin King
Cc: Joe Perches, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
2018-03-26 15:47 ` Ajay Singh
@ 2018-03-26 16:15 ` Colin Ian King
-1 siblings, 0 replies; 9+ messages in thread
From: Colin Ian King @ 2018-03-26 16:15 UTC (permalink / raw)
To: Ajay Singh
Cc: Joe Perches, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel, kernel-janitors, linux-kernel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
@ 2018-03-26 16:15 ` Colin Ian King
0 siblings, 0 replies; 9+ messages in thread
From: Colin Ian King @ 2018-03-26 16:15 UTC (permalink / raw)
To: Ajay Singh
Cc: Joe Perches, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel, kernel-janitors, linux-kernel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: wilc1000: check for kmalloc allocation failures
2018-03-21 19:19 ` Colin King
(?)
(?)
@ 2018-03-22 6:47 ` Walter Harms
-1 siblings, 0 replies; 9+ messages in thread
From: Walter Harms @ 2018-03-22 6:47 UTC (permalink / raw)
To: kernel-janitors
Am 21.03.2018 20:19, schrieb 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);
> }
>
Perhaps these malloc+memcpy can be replaced with a single kmemdup() ?
re,
wh
> @@ -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);
^ permalink raw reply [flat|nested] 9+ messages in thread