diff mbox series

net/ice: fix possible memory leak

Message ID 20240711165907.3169191-1-vladimir.medvedkin@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Bruce Richardson
Headers
Series net/ice: fix possible memory leak |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

Vladimir Medvedkin July 11, 2024, 4:59 p.m. UTC 
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.

Fixes: 1b9c68120a1c ("net/ice: enable protocol agnostic flow offloading in RSS")
Cc: ting.xu@intel.com
Cc:stable@dpdk.org

Reported-by: Michael Theodore Stolarchuk <mike.stolarchuk@arista.com>
Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
---
 drivers/net/ice/ice_hash.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Comments

Bruce Richardson July 11, 2024, 5:05 p.m. UTC | #1 
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@intel.com
> Cc:stable@dpdk.org
> 
> Reported-by: Michael Theodore Stolarchuk <mike.stolarchuk@arista.com>
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@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
>
Vladimir Medvedkin July 11, 2024, 5:07 p.m. UTC | #2 
Hi Bruce,

Sure, will submit v2.

Thanks!

-----Original Message-----
From: Richardson, Bruce <bruce.richardson@intel.com> 
Sent: Thursday, July 11, 2024 6:05 PM
To: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
Cc: dev@dpdk.org; ting.xu@intel.com; Stolarchuk, Michael <mike.stolarchuk@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@intel.com
> Cc:stable@dpdk.org
> 
> Reported-by: Michael Theodore Stolarchuk <mike.stolarchuk@arista.com>
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@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
>
diff mbox series

Patch

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