[PATCH] i40e: fix the issue reported by klocwork

[PATCH] i40e: fix the issue reported by klocwork

[Jingjing Wu]

Klocwork reports array 'src_offset' may use index 16.
In function i40e_srcoff_to_flx_pit, index j + 1 can reach I40E_FDIR_MAX_FLEX_LEN.
This patch fixes this issue to avoid array bound.

Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_i40e/i40e_fdir.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_fdir.c b/lib/librte_pmd_i40e/i40e_fdir.c
index 68511c8..bc36d8e 100644
--- a/lib/librte_pmd_i40e/i40e_fdir.c
+++ b/lib/librte_pmd_i40e/i40e_fdir.c
@@ -402,28 +402,27 @@ i40e_srcoff_to_flx_pit(const uint16_t *src_offset,
 
 	while (j < I40E_FDIR_MAX_FLEX_LEN) {
 		size = 1;
-		for (; j < I40E_FDIR_MAX_FLEX_LEN; j++) {
+		for (; j < I40E_FDIR_MAX_FLEX_LEN - 1; j++) {
 			if (src_offset[j + 1] == src_offset[j] + 1)
 				size++;
-			else {
-				src_tmp = src_offset[j] + 1 - size;
-				/* the flex_pit need to be sort by scr_offset */
-				for (i = 0; i < num; i++) {
-					if (src_tmp < flex_pit[i].src_offset)
-						break;
-				}
-				/* if insert required, move backward */
-				for (k = num; k > i; k--)
-					flex_pit[k] = flex_pit[k - 1];
-				/* insert */
-				flex_pit[i].dst_offset = j + 1 - size;
-				flex_pit[i].src_offset = src_tmp;
-				flex_pit[i].size = size;
-				j++;
-				num++;
+			else
+				break;
+		}
+		src_tmp = src_offset[j] + 1 - size;
+		/* the flex_pit need to be sort by src_offset */
+		for (i = 0; i < num; i++) {
+			if (src_tmp < flex_pit[i].src_offset)
 				break;
-			}
 		}
+		/* if insert required, move backward */
+		for (k = num; k > i; k--)
+			flex_pit[k] = flex_pit[k - 1];
+		/* insert */
+		flex_pit[i].dst_offset = j + 1 - size;
+		flex_pit[i].src_offset = src_tmp;
+		flex_pit[i].size = size;
+		j++;
+		num++;
 	}
 	return num;
 }
-- 
1.9.3

Re: [PATCH] i40e: fix the issue reported by klocwork

[Thomas Monjalon]

Helin, is this patch valid and important?

2015-02-12 19:22, Jingjing Wu:
> Klocwork reports array 'src_offset' may use index 16.
> In function i40e_srcoff_to_flx_pit, index j + 1 can reach I40E_FDIR_MAX_FLEX_LEN.
> This patch fixes this issue to avoid array bound.
> 
> Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  lib/librte_pmd_i40e/i40e_fdir.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_pmd_i40e/i40e_fdir.c b/lib/librte_pmd_i40e/i40e_fdir.c
> index 68511c8..bc36d8e 100644
> --- a/lib/librte_pmd_i40e/i40e_fdir.c
> +++ b/lib/librte_pmd_i40e/i40e_fdir.c
> @@ -402,28 +402,27 @@ i40e_srcoff_to_flx_pit(const uint16_t *src_offset,
>  
>  	while (j < I40E_FDIR_MAX_FLEX_LEN) {
>  		size = 1;
> -		for (; j < I40E_FDIR_MAX_FLEX_LEN; j++) {
> +		for (; j < I40E_FDIR_MAX_FLEX_LEN - 1; j++) {
>  			if (src_offset[j + 1] == src_offset[j] + 1)
>  				size++;
> -			else {
> -				src_tmp = src_offset[j] + 1 - size;
> -				/* the flex_pit need to be sort by scr_offset */
> -				for (i = 0; i < num; i++) {
> -					if (src_tmp < flex_pit[i].src_offset)
> -						break;
> -				}
> -				/* if insert required, move backward */
> -				for (k = num; k > i; k--)
> -					flex_pit[k] = flex_pit[k - 1];
> -				/* insert */
> -				flex_pit[i].dst_offset = j + 1 - size;
> -				flex_pit[i].src_offset = src_tmp;
> -				flex_pit[i].size = size;
> -				j++;
> -				num++;
> +			else
> +				break;
> +		}
> +		src_tmp = src_offset[j] + 1 - size;
> +		/* the flex_pit need to be sort by src_offset */
> +		for (i = 0; i < num; i++) {
> +			if (src_tmp < flex_pit[i].src_offset)
>  				break;
> -			}
>  		}
> +		/* if insert required, move backward */
> +		for (k = num; k > i; k--)
> +			flex_pit[k] = flex_pit[k - 1];
> +		/* insert */
> +		flex_pit[i].dst_offset = j + 1 - size;
> +		flex_pit[i].src_offset = src_tmp;
> +		flex_pit[i].size = size;
> +		j++;
> +		num++;
>  	}
>  	return num;
>  }
> 

Re: [PATCH] i40e: fix the issue reported by klocwork

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Jingjing Wu
> Sent: Thursday, February 12, 2015 7:22 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH] i40e: fix the issue reported by klocwork
> 
> Klocwork reports array 'src_offset' may use index 16.
> In function i40e_srcoff_to_flx_pit, index j + 1 can reach
> I40E_FDIR_MAX_FLEX_LEN.
> This patch fixes this issue to avoid array bound.
> 
> Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

> ---
>  lib/librte_pmd_i40e/i40e_fdir.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)

Re: [PATCH] i40e: fix the issue reported by klocwork

[Cao, Min]

Tested-by: Min Cao <min.cao-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Patch name: 		[dpdk-dev] [PATCH] i40e: fix the issue reported by klocwork
Test Flag: 			Tested-by
Tester name: 		min.cao-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Result summary:		total 2 cases, 2passed, 0 failed

Test Case 1:		
Name:				ipv4 fwd
Environment:		OS: Fedora20 3.11.10-301.fc20.x86_64
				gcc (GCC) 4.8.2
				CPU: Intel(R) Xeon(R) CPU E5-2680 0 @ 2.70GHz
				NIC: Fortville eagle 
Test result:		PASSED
Detail:                 ipv4 fwd

Test Case 2:		
Name:				ipv6 fwd
Environment:		OS: Fedora20 3.11.10-301.fc20.x86_64
				gcc (GCC) 4.8.2
				CPU: Intel(R) Xeon(R) CPU E5-2680 0 @ 2.70GHz
				NIC: Fortville eagle 
Test result:		PASSED
Detail:                 ipv6 fwd		

-----Original Message-----
From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Jingjing Wu
Sent: Thursday, February 12, 2015 7:22 PM
To: dev-VfR2kkLFssw@public.gmane.org
Subject: [dpdk-dev] [PATCH] i40e: fix the issue reported by klocwork

Klocwork reports array 'src_offset' may use index 16.
In function i40e_srcoff_to_flx_pit, index j + 1 can reach I40E_FDIR_MAX_FLEX_LEN.
This patch fixes this issue to avoid array bound.

Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_i40e/i40e_fdir.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_fdir.c b/lib/librte_pmd_i40e/i40e_fdir.c
index 68511c8..bc36d8e 100644
--- a/lib/librte_pmd_i40e/i40e_fdir.c
+++ b/lib/librte_pmd_i40e/i40e_fdir.c
@@ -402,28 +402,27 @@ i40e_srcoff_to_flx_pit(const uint16_t *src_offset,
 
 	while (j < I40E_FDIR_MAX_FLEX_LEN) {
 		size = 1;
-		for (; j < I40E_FDIR_MAX_FLEX_LEN; j++) {
+		for (; j < I40E_FDIR_MAX_FLEX_LEN - 1; j++) {
 			if (src_offset[j + 1] == src_offset[j] + 1)
 				size++;
-			else {
-				src_tmp = src_offset[j] + 1 - size;
-				/* the flex_pit need to be sort by scr_offset */
-				for (i = 0; i < num; i++) {
-					if (src_tmp < flex_pit[i].src_offset)
-						break;
-				}
-				/* if insert required, move backward */
-				for (k = num; k > i; k--)
-					flex_pit[k] = flex_pit[k - 1];
-				/* insert */
-				flex_pit[i].dst_offset = j + 1 - size;
-				flex_pit[i].src_offset = src_tmp;
-				flex_pit[i].size = size;
-				j++;
-				num++;
+			else
+				break;
+		}
+		src_tmp = src_offset[j] + 1 - size;
+		/* the flex_pit need to be sort by src_offset */
+		for (i = 0; i < num; i++) {
+			if (src_tmp < flex_pit[i].src_offset)
 				break;
-			}
 		}
+		/* if insert required, move backward */
+		for (k = num; k > i; k--)
+			flex_pit[k] = flex_pit[k - 1];
+		/* insert */
+		flex_pit[i].dst_offset = j + 1 - size;
+		flex_pit[i].src_offset = src_tmp;
+		flex_pit[i].size = size;
+		j++;
+		num++;
 	}
 	return num;
 }
-- 
1.9.3

Re: [PATCH] i40e: fix the issue reported by klocwork

[Thomas Monjalon]

Hi Helin,

> > Klocwork reports array 'src_offset' may use index 16.
> > In function i40e_srcoff_to_flx_pit, index j + 1 can reach
> > I40E_FDIR_MAX_FLEX_LEN.
> > This patch fixes this issue to avoid array bound.
> > 
> > Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Acked-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Please confirm it's a real bug which needs to be fixed in 2.0,
and/or you are sure this patch won't bring a new problem.

Thanks

Re: [PATCH] i40e: fix the issue reported by klocwork

[Zhang, Helin]

Hi Thomas

Actually it is a bug fix. It would be better to be put in R2.0.
It may not crash, as it just possibly read something out of range. I am waiting the test report from our validation team, and then I will merge that. Thanks for your patience!

Regards,
Helin

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Tuesday, March 31, 2015 6:28 PM
> To: Zhang, Helin
> Cc: dev-VfR2kkLFssw@public.gmane.org; Wu, Jingjing
> Subject: Re: [dpdk-dev] [PATCH] i40e: fix the issue reported by klocwork
> 
> Hi Helin,
> 
> > > Klocwork reports array 'src_offset' may use index 16.
> > > In function i40e_srcoff_to_flx_pit, index j + 1 can reach
> > > I40E_FDIR_MAX_FLEX_LEN.
> > > This patch fixes this issue to avoid array bound.
> > >
> > > Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Acked-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Please confirm it's a real bug which needs to be fixed in 2.0, and/or you are sure
> this patch won't bring a new problem.
> 
> Thanks

Re: [PATCH] i40e: fix the issue reported by klocwork

[Thomas Monjalon]

> > Klocwork reports array 'src_offset' may use index 16.
> > In function i40e_srcoff_to_flx_pit, index j + 1 can reach
> > I40E_FDIR_MAX_FLEX_LEN.
> > This patch fixes this issue to avoid array bound.
> > 
> > Signed-off-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Acked-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Fixes: d8b90c4eabe9 ("i40e: take flow director flexible payload configuration")

Applied, thanks