Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] staging: wilc1000: replace kmalloc + memcpy with kmemdup
To:     Colin King <colin.king@canonical.com>,
        Aditya Shankar <aditya.shankar@microchip.com>,
        Ganesh Krishna <ganesh.krishna@microchip.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-wireless@vger.kernel.org>, <devel@driverdev.osuosl.org>
CC:     <kernel-janitors@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20180326171629.28700-1-colin.king@canonical.com>
From:   Claudiu Beznea <Claudiu.Beznea@microchip.com>
Message-ID: <f674c9e0-e67f-885f-cdd8-ce5da1fcc262@microchip.com>
Date:   Tue, 27 Mar 2018 11:44:18 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <20180326171629.28700-1-colin.king@canonical.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 26.03.2018 20:16, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Replace several allocation and memcpys with kmemdup and add in some
> missing memory allocation failure checks.  Also fix an incorrect 
> -EFAULT return with -ENOMEM.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 75 +++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 9b9b86654958..8fd367f87fa5 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -797,6 +797,10 @@ static s32 handle_scan(struct wilc_vif *vif, struct scan_attr *scan_info)
>  	for (i = 0; i < hidden_net->n_ssids; i++)
>  		valuesize += ((hidden_net->net_info[i].ssid_len) + 1);
>  	hdn_ntwk_wid_val = kmalloc(valuesize + 1, GFP_KERNEL);
> +	if (!hdn_ntwk_wid_val) {
> +		result = -ENOMEM;
> +		goto error;
> +	}
>  	wid_list[index].val = hdn_ntwk_wid_val;
>  	if (wid_list[index].val) {
>  		buffer = wid_list[index].val;
> @@ -943,39 +947,35 @@ static s32 handle_connect(struct wilc_vif *vif,
>  	}
>  
>  	if (conn_attr->bssid) {

These:
> -		hif_drv->usr_conn_req.bssid = kmalloc(6, GFP_KERNEL);
> +		hif_drv->usr_conn_req.bssid = kmemdup(conn_attr->bssid, 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);
>  	}
>  
>  	hif_drv->usr_conn_req.ssid_len = conn_attr->ssid_len;
>  	if (conn_attr->ssid) {
> -		hif_drv->usr_conn_req.ssid = kmalloc(conn_attr->ssid_len + 1,
> +		hif_drv->usr_conn_req.ssid = kmemdup(conn_attr->ssid,
> +						     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);
>  		hif_drv->usr_conn_req.ssid[conn_attr->ssid_len] = '\0';
>  	}
>  
>  	hif_drv->usr_conn_req.ies_len = conn_attr->ies_len;
>  	if (conn_attr->ies) {

and these:
> -		hif_drv->usr_conn_req.ies = kmalloc(conn_attr->ies_len,
> +		hif_drv->usr_conn_req.ies = kmemdup(conn_attr->ies,
> +						    conn_attr->ies_len,
>  						    GFP_KERNEL);

were already done in [1]. I don't know which was the first...

[1] https://patchwork.kernel.org/patch/10308575/

>  		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);
>  	}
>  
>  	hif_drv->usr_conn_req.security = conn_attr->security;
> @@ -1009,9 +1009,12 @@ static s32 handle_connect(struct wilc_vif *vif,
>  
>  	if (memcmp("DIRECT-", conn_attr->ssid, 7)) {
>  		info_element_size = hif_drv->usr_conn_req.ies_len;
> -		info_element = kmalloc(info_element_size, GFP_KERNEL);
> -		memcpy(info_element, hif_drv->usr_conn_req.ies,
> -		       info_element_size);
> +		info_element = kmemdup(hif_drv->usr_conn_req.ies,
> +				       info_element_size, GFP_KERNEL);
> +		if (!info_element) {
> +			result = -ENOMEM;
> +			goto error;
> +		}
>  	}
>  	wid_list[wid_cnt].id = (u16)WID_11I_MODE;
>  	wid_list[wid_cnt].type = WID_CHAR;
> @@ -1039,9 +1042,13 @@ static s32 handle_connect(struct wilc_vif *vif,
>  	if (memcmp("DIRECT-", conn_attr->ssid, 7)) {
>  		join_req_size = wid_list[wid_cnt].size;
>  		join_req = kmalloc(join_req_size, GFP_KERNEL);
> +		if (!join_req) {
> +			result = -ENOMEM;
> +			goto error;
> +		}
>  	}
>  	if (!wid_list[wid_cnt].val) {
> -		result = -EFAULT;
> +		result = -ENOMEM;
>  		goto error;
>  	}
>  
> @@ -1166,11 +1173,13 @@ static s32 handle_connect(struct wilc_vif *vif,
>  
>  			if (conn_attr->ies) {
>  				conn_info.req_ies_len = conn_attr->ies_len;
> -				conn_info.req_ies = kmalloc(conn_attr->ies_len,
> +				conn_info.req_ies = kmemdup(conn_attr->ies,
> +							    conn_attr->ies_len,
>  							    GFP_KERNEL);
> -				memcpy(conn_info.req_ies,
> -				       conn_attr->ies,
> -				       conn_attr->ies_len);
> +				if (!conn_info.req_ies) {
> +					result = -ENOMEM;
> +					goto error_free;
> +				}
>  			}
>  
>  			conn_attr->result(CONN_DISCONN_EVENT_CONN_RESP,
> @@ -1187,6 +1196,7 @@ static s32 handle_connect(struct wilc_vif *vif,
>  		}
>  	}
>  
> +error_free:
>  	kfree(conn_attr->bssid);
>  	conn_attr->bssid = NULL;
>  
> @@ -1227,10 +1237,9 @@ static s32 handle_connect_timeout(struct wilc_vif *vif)
>  
>  		if (hif_drv->usr_conn_req.ies) {
>  			info.req_ies_len = hif_drv->usr_conn_req.ies_len;
> -			info.req_ies = kmalloc(hif_drv->usr_conn_req.ies_len, GFP_KERNEL);
> -			memcpy(info.req_ies,
> -			       hif_drv->usr_conn_req.ies,
> -			       hif_drv->usr_conn_req.ies_len);
> +			info.req_ies = kmemdup(hif_drv->usr_conn_req.ies,
> +					       hif_drv->usr_conn_req.ies_len,
> +					       GFP_KERNEL);
>  		}
>  
>  		hif_drv->usr_conn_req.conn_result(CONN_DISCONN_EVENT_CONN_RESP,
> @@ -1421,9 +1430,11 @@ static s32 handle_rcvd_gnrl_async_info(struct wilc_vif *vif,
>  
>  						if (conn_info.status == SUCCESSFUL_STATUSCODE && connect_resp_info->ies) {
>  							conn_info.resp_ies_len = connect_resp_info->ies_len;
> -							conn_info.resp_ies = kmalloc(connect_resp_info->ies_len, GFP_KERNEL);
> -							memcpy(conn_info.resp_ies, connect_resp_info->ies,
> -							       connect_resp_info->ies_len);
> +							conn_info.resp_ies = kmemdup(connect_resp_info->ies,
> +										     connect_resp_info->ies_len,
> +										     GFP_KERNEL);
> +							if (!conn_info.resp_ies)
> +								return -ENOMEM;
>  						}
>  
>  						if (connect_resp_info) {
> @@ -3524,8 +3535,11 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
>  	msg.vif = vif;
>  
>  	msg.body.net_info.len = length;
> -	msg.body.net_info.buffer = kmalloc(length, GFP_KERNEL);
> -	memcpy(msg.body.net_info.buffer, buffer, length);
> +	msg.body.net_info.buffer = kmemdup(buffer, length, GFP_KERNEL);
> +	if (!msg.body.net_info.buffer) {
> +		netdev_err(vif->ndev, "message buffer allocation failed\n");
> +		return;
> +	}
>  
>  	result = wilc_enqueue_cmd(&msg);
>  	if (result)
> @@ -3571,8 +3585,11 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
>  	msg.vif = vif;
>  
>  	msg.body.async_info.len = length;
> -	msg.body.async_info.buffer = kmalloc(length, GFP_KERNEL);
> -	memcpy(msg.body.async_info.buffer, buffer, length);
> +	msg.body.async_info.buffer = kmemdup(buffer, length, GFP_KERNEL);
> +	if (!msg.body.async_info.buffer) {
> +		netdev_err(vif->ndev, "message buffer allocation failed\n");
> +		return;
> +	}
>  
>  	result = wilc_enqueue_cmd(&msg);
>  	if (result)
>