[PATCH] net/ice: fix possible memory leak

[Medvedkin, Vladimir]

Hi Bruce,

Sure, will submit v2.

Thanks!

-----Original Message-----
From: Richardson, Bruce <bruce.richardson at intel.com> 
Sent: Thursday, July 11, 2024 6:05 PM
To: Medvedkin, Vladimir <vladimir.medvedkin at intel.com>
Cc: dev at dpdk.org; ting.xu at intel.com; Stolarchuk, Michael <mike.stolarchuk at arista.com>
Subject: Re: [PATCH] net/ice: fix possible memory leak

On Thu, Jul 11, 2024 at 04:59:07PM +0000, Vladimir Medvedkin wrote:
> This patch fixes possible memleak inside the 
> ice_hash_parse_raw_pattern().
> Additionally replaces using strlen() with more secure strnlen().
> Also replaces the returned inconsistent rte_errno with explicit error 
> statuses.
> 

Hi Vladimir,

I think you could do with explaining more about how the memory leak comes about. It's also not helped by having multiple additional changes in this patch, can they be split off into a separate patch to fix the error codes (and change the strlen in passing), or even two additional patches for the error code and strlen issues individually.

/Bruce.

> Fixes: 1b9c68120a1c ("net/ice: enable protocol agnostic flow 
> offloading in RSS")
> Cc: ting.xu at intel.com
> Cc:stable at dpdk.org
> 
> Reported-by: Michael Theodore Stolarchuk <mike.stolarchuk at arista.com>
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin at intel.com>
> ---
>  drivers/net/ice/ice_hash.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c 
> index f923641533..a7dbb54d91 100644
> --- a/drivers/net/ice/ice_hash.c
> +++ b/drivers/net/ice/ice_hash.c
> @@ -650,18 +650,18 @@ ice_hash_parse_raw_pattern(struct ice_adapter *ad,
>  	uint8_t *pkt_buf, *msk_buf;
>  	uint8_t tmp_val = 0;
>  	uint8_t tmp_c = 0;
> -	int i, j;
> +	int i, j, ret = 0;
>  
>  	if (ad->psr == NULL)
> -		return -rte_errno;
> +		return -ENOTSUP;
>  
>  	raw_spec = item->spec;
>  	raw_mask = item->mask;
>  
> -	spec_len = strlen((char *)(uintptr_t)raw_spec->pattern);
> -	if (strlen((char *)(uintptr_t)raw_mask->pattern) !=
> -		spec_len)
> -		return -rte_errno;
> +	spec_len = strnlen((char *)(uintptr_t)raw_spec->pattern, raw_spec->length);
> +	if (strnlen((char *)(uintptr_t)raw_mask->pattern, raw_mask->length) !=
> +			spec_len)
> +		return -EINVAL;
>  
>  	pkt_len = spec_len / 2;
>  
> @@ -670,8 +670,10 @@ ice_hash_parse_raw_pattern(struct ice_adapter *ad,
>  		return -ENOMEM;
>  
>  	msk_buf = rte_zmalloc(NULL, pkt_len, 0);
> -	if (!msk_buf)
> +	if (!msk_buf) {
> +		rte_free(pkt_buf);
>  		return -ENOMEM;
> +	}
>  
>  	/* convert string to int array */
>  	for (i = 0, j = 0; i < spec_len; i += 2, j++) { @@ -708,18 +710,22 
> @@ ice_hash_parse_raw_pattern(struct ice_adapter *ad,
>  			msk_buf[j] = tmp_val * 16 + tmp_c - '0';
>  	}
>  
> -	if (ice_parser_run(ad->psr, pkt_buf, pkt_len, &rslt))
> -		return -rte_errno;
> +	ret = ice_parser_run(ad->psr, pkt_buf, pkt_len, &rslt);
> +	if (ret)
> +		goto free_mem;
>  
> -	if (ice_parser_profile_init(&rslt, pkt_buf, msk_buf,
> -		pkt_len, ICE_BLK_RSS, true, &prof))
> -		return -rte_errno;
> +	ret = ice_parser_profile_init(&rslt, pkt_buf, msk_buf,
> +		pkt_len, ICE_BLK_RSS, true, &prof);
> +	if (ret)
> +		goto free_mem;
>  
>  	rte_memcpy(&meta->raw.prof, &prof, sizeof(prof));
>  
> +free_mem:
>  	rte_free(pkt_buf);
>  	rte_free(msk_buf);
> -	return 0;
> +
> +	return ret;
>  }
>  
>  static void
> --
> 2.34.1
>