| Message ID | 20180321191941.4126-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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);
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
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 >
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);