DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects
@ 2016-06-30  7:34 Beilei Xing
  2016-07-04 13:58 ` Bruce Richardson
  2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
  0 siblings, 2 replies; 13+ messages in thread
From: Beilei Xing @ 2016-06-30  7:34 UTC (permalink / raw)
  To: jingjing.wu, michalx.k.jastrzebski; +Cc: dev

Fix some open coverity defects.

Beilei Xing (4):
  i40e: fix wrong operator
  i40e: fix dereference before null check
  i40e: fix out-of-bounds access
  examples/tep_term: fix out-of-bounds access

 drivers/net/i40e/i40e_ethdev.c         | 35 ++++++++++++++++++++--------------
 drivers/net/i40e/i40e_ethdev_vf.c      |  7 +++++--
 examples/tep_termination/vxlan_setup.c |  2 +-
 3 files changed, 27 insertions(+), 17 deletions(-)

-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects
  2016-06-30  7:34 [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects Beilei Xing
@ 2016-07-04 13:58 ` Bruce Richardson
  2016-07-05  1:47   ` Xing, Beilei
  2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
  1 sibling, 1 reply; 13+ messages in thread
From: Bruce Richardson @ 2016-07-04 13:58 UTC (permalink / raw)
  To: Beilei Xing; +Cc: jingjing.wu, michalx.k.jastrzebski, dev

On Thu, Jun 30, 2016 at 03:34:16PM +0800, Beilei Xing wrote:
> Fix some open coverity defects.
> 
> Beilei Xing (4):
>   i40e: fix wrong operator
>   i40e: fix dereference before null check
>   i40e: fix out-of-bounds access
>   examples/tep_term: fix out-of-bounds access
> 
>  drivers/net/i40e/i40e_ethdev.c         | 35 ++++++++++++++++++++--------------
>  drivers/net/i40e/i40e_ethdev_vf.c      |  7 +++++--
>  examples/tep_termination/vxlan_setup.c |  2 +-
>  3 files changed, 27 insertions(+), 17 deletions(-)
> 
> -- 

Hi Beilei,

thanks for the patches. Since these are coverity fixes, can you please do a V2
with the coverity id's being fixed called out in the commit message. The format
to use is:

	Coverity issue: xxxxx yyyyy ....

just above the fixes line in the patch. [Use "git log" to see examples in other
commits].

Also, for convenience sake for myself and Thomas, can you include just the three
i40e patches in one set - since those go to next-net tree - and submit the
examples fix as a separate standalone patch to go in the main tree.

Thanks,
/Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects
  2016-07-04 13:58 ` Bruce Richardson
@ 2016-07-05  1:47   ` Xing, Beilei
  0 siblings, 0 replies; 13+ messages in thread
From: Xing, Beilei @ 2016-07-05  1:47 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: Wu, Jingjing, Jastrzebski, MichalX K, dev

Hi Bruce,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, July 4, 2016 9:58 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Jastrzebski, MichalX K
> <michalx.k.jastrzebski@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects
> 
> On Thu, Jun 30, 2016 at 03:34:16PM +0800, Beilei Xing wrote:
> > Fix some open coverity defects.
> >
> > Beilei Xing (4):
> >   i40e: fix wrong operator
> >   i40e: fix dereference before null check
> >   i40e: fix out-of-bounds access
> >   examples/tep_term: fix out-of-bounds access
> >
> >  drivers/net/i40e/i40e_ethdev.c         | 35 ++++++++++++++++++++---------
> -----
> >  drivers/net/i40e/i40e_ethdev_vf.c      |  7 +++++--
> >  examples/tep_termination/vxlan_setup.c |  2 +-
> >  3 files changed, 27 insertions(+), 17 deletions(-)
> >
> > --
> 
> Hi Beilei,
> 
> thanks for the patches. Since these are coverity fixes, can you please do a V2
> with the coverity id's being fixed called out in the commit message. The
> format to use is:
> 
> 	Coverity issue: xxxxx yyyyy ....
> 
> just above the fixes line in the patch. [Use "git log" to see examples in other
> commits].

Thanks for guides:)

> 
> Also, for convenience sake for myself and Thomas, can you include just the
> three i40e patches in one set - since those go to next-net tree - and submit
> the examples fix as a separate standalone patch to go in the main tree.

OK, no problem, will send v2 later.

> 
> Thanks,
> /Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v2 0/3] fix coverity defects
  2016-06-30  7:34 [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects Beilei Xing
  2016-07-04 13:58 ` Bruce Richardson
@ 2016-07-05  6:10 ` Beilei Xing
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 1/3] i40e: fix log error Beilei Xing
                     ` (3 more replies)
  1 sibling, 4 replies; 13+ messages in thread
From: Beilei Xing @ 2016-07-05  6:10 UTC (permalink / raw)
  To: jingjing.wu, michalx.k.jastrzebski; +Cc: dev

Fix some open coverity defects.

V2 changes:
 Rework commit log.
 Refactor patchset.

Beilei Xing (3):
  i40e: fix log error
  i40e: fix dereference before null check
  i40e: fix out-of-bounds access

 drivers/net/i40e/i40e_ethdev.c    | 35 +++++++++++++++++++++--------------
 drivers/net/i40e/i40e_ethdev_vf.c |  7 +++++--
 2 files changed, 26 insertions(+), 16 deletions(-)

-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v2 1/3] i40e: fix log error
  2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
@ 2016-07-05  6:10   ` Beilei Xing
  2016-07-05 13:24     ` Bruce Richardson
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 2/3] i40e: fix dereference before null check Beilei Xing
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Beilei Xing @ 2016-07-05  6:10 UTC (permalink / raw)
  To: jingjing.wu, michalx.k.jastrzebski; +Cc: dev, Beilei Xing

The condition, "(pf->flags | I40E_FLAG_VMDQ)" will always be true,
regardless of the value of the flags operand, because I40E_FLAG_VMDQ
is 4ULL - meaning at least one bit will always be set in the result.
That will cause log error when VMDq is disabled.
Since the original intent behind the condition is to check if VMDq
is enabled, fix the code by changing "|" to "&".

Coverity issue: 13219, 13221

Fixes: 4805ed59e957 ("i40e: enhance mac address operations")

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index f414d93..46ae866 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2952,9 +2952,10 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
 	int ret;
 
 	/* If VMDQ not enabled or configured, return */
-	if (pool != 0 && (!(pf->flags | I40E_FLAG_VMDQ) || !pf->nb_cfg_vmdq_vsi)) {
+	if (pool != 0 && (!(pf->flags & I40E_FLAG_VMDQ) ||
+			  !pf->nb_cfg_vmdq_vsi)) {
 		PMD_DRV_LOG(ERR, "VMDQ not %s, can't set mac to pool %u",
-			pf->flags | I40E_FLAG_VMDQ ? "configured" : "enabled",
+			pf->flags & I40E_FLAG_VMDQ ? "configured" : "enabled",
 			pool);
 		return;
 	}
@@ -3005,7 +3006,7 @@ i40e_macaddr_remove(struct rte_eth_dev *dev, uint32_t index)
 				vsi = pf->main_vsi;
 			else {
 				/* No VMDQ pool enabled or configured */
-				if (!(pf->flags | I40E_FLAG_VMDQ) ||
+				if (!(pf->flags & I40E_FLAG_VMDQ) ||
 					(i > pf->nb_cfg_vmdq_vsi)) {
 					PMD_DRV_LOG(ERR, "No VMDQ pool enabled"
 							"/configured");
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v2 2/3] i40e: fix dereference before null check
  2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 1/3] i40e: fix log error Beilei Xing
@ 2016-07-05  6:10   ` Beilei Xing
  2016-07-05 13:24     ` Bruce Richardson
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access Beilei Xing
  2016-07-08 15:33   ` [dpdk-dev] [PATCH v2 0/3] fix coverity defects Bruce Richardson
  3 siblings, 1 reply; 13+ messages in thread
From: Beilei Xing @ 2016-07-05  6:10 UTC (permalink / raw)
  To: jingjing.wu, michalx.k.jastrzebski; +Cc: dev, Beilei Xing

Null-checking vsi suggests that it may be null, but it
has be dereferenced before null-checking. So move if
statement to the front of assignment statement.

Coverity: 119265, 119266

Fixes: d0a349409bd7 ("i40e: support AQ based RSS config")
Fixes: 647d1eaf758b ("i40evf: support AQ based RSS config")

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c    | 7 +++++--
 drivers/net/i40e/i40e_ethdev_vf.c | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 46ae866..a1cad37 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3168,13 +3168,16 @@ i40e_get_rss_lut(struct i40e_vsi *vsi, uint8_t *lut, uint16_t lut_size)
 static int
 i40e_set_rss_lut(struct i40e_vsi *vsi, uint8_t *lut, uint16_t lut_size)
 {
-	struct i40e_pf *pf = I40E_VSI_TO_PF(vsi);
-	struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
+	struct i40e_pf *pf;
+	struct i40e_hw *hw;
 	int ret;
 
 	if (!vsi || !lut)
 		return -EINVAL;
 
+	pf = I40E_VSI_TO_PF(vsi);
+	hw = I40E_VSI_TO_HW(vsi);
+
 	if (pf->flags & I40E_FLAG_RSS_AQ_CAPABLE) {
 		ret = i40e_aq_set_rss_lut(hw, vsi->vsi_id, TRUE,
 					  lut, lut_size);
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 7b6df1d..d727232 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -2377,13 +2377,16 @@ i40evf_get_rss_lut(struct i40e_vsi *vsi, uint8_t *lut, uint16_t lut_size)
 static int
 i40evf_set_rss_lut(struct i40e_vsi *vsi, uint8_t *lut, uint16_t lut_size)
 {
-	struct i40e_vf *vf = I40E_VSI_TO_VF(vsi);
-	struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
+	struct i40e_vf *vf;
+	struct i40e_hw *hw;
 	int ret;
 
 	if (!vsi || !lut)
 		return -EINVAL;
 
+	vf = I40E_VSI_TO_VF(vsi);
+	hw = I40E_VSI_TO_HW(vsi);
+
 	if (vf->flags & I40E_FLAG_RSS_AQ_CAPABLE) {
 		ret = i40e_aq_set_rss_lut(hw, vsi->vsi_id, FALSE,
 					  lut, lut_size);
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access
  2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 1/3] i40e: fix log error Beilei Xing
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 2/3] i40e: fix dereference before null check Beilei Xing
@ 2016-07-05  6:10   ` Beilei Xing
  2016-07-05 13:26     ` Bruce Richardson
  2016-07-08 15:33   ` [dpdk-dev] [PATCH v2 0/3] fix coverity defects Bruce Richardson
  3 siblings, 1 reply; 13+ messages in thread
From: Beilei Xing @ 2016-07-05  6:10 UTC (permalink / raw)
  To: jingjing.wu, michalx.k.jastrzebski; +Cc: dev, Beilei Xing

When calling i40e_flowtype_to_pctype in
i40e_get_hash_filter_global_config and
i40e_set_hash_filter_global_config, function
i40e_flowtype_to_pctype will be possibly
out-of-bounds accessed, because size of callee's array
is 15. So judge flow type before calling
i40e_flowtype_to_pctype.
Meanwhile do the same change in other functions.

Coverity issue: 37793, 37794

Fixes: 782c8c92f13f ("i40e: add hash configuration")
Fixes: f2b2e2354bbd ("i40e: split function for hash and flow director input")
Fixes: 98f055707685 ("i40e: configure input fields for RSS or flow director")

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index a1cad37..111a552 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -6908,6 +6908,9 @@ i40e_get_hash_filter_global_config(struct i40e_hw *hw,
 		mask &= ~(1UL << i);
 		/* Bit set indicats the coresponding flow type is supported */
 		g_cfg->valid_bit_mask[0] |= (1UL << i);
+		/* if flowtype is invalid, continue */
+		if (!I40E_VALID_FLOW(i))
+			continue;
 		pctype = i40e_flowtype_to_pctype(i);
 		reg = i40e_read_rx_ctl(hw, I40E_GLQF_HSYM(pctype));
 		if (reg & I40E_GLQF_HSYM_SYMH_ENA_MASK)
@@ -6979,6 +6982,9 @@ i40e_set_hash_filter_global_config(struct i40e_hw *hw,
 		if (!(mask0 & (1UL << i)))
 			continue;
 		mask0 &= ~(1UL << i);
+		/* if flowtype is invalid, continue */
+		if (!I40E_VALID_FLOW(i))
+			continue;
 		pctype = i40e_flowtype_to_pctype(i);
 		reg = (g_cfg->sym_hash_enable_mask[0] & (1UL << i)) ?
 				I40E_GLQF_HSYM_SYMH_ENA_MASK : 0;
@@ -7541,13 +7547,11 @@ i40e_hash_filter_inset_select(struct i40e_hw *hw,
 		return -EINVAL;
 	}
 
-	pctype = i40e_flowtype_to_pctype(conf->flow_type);
-	if (pctype == 0 || pctype > I40E_FILTER_PCTYPE_L2_PAYLOAD) {
-		PMD_DRV_LOG(ERR, "Not supported flow type (%u)",
-			    conf->flow_type);
+	if (!I40E_VALID_FLOW(conf->flow_type)) {
+		PMD_DRV_LOG(ERR, "invalid flow_type input.");
 		return -EINVAL;
 	}
-
+	pctype = i40e_flowtype_to_pctype(conf->flow_type);
 	ret = i40e_parse_input_set(&input_set, pctype, conf->field,
 				   conf->inset_size);
 	if (ret) {
@@ -7612,12 +7616,11 @@ i40e_fdir_filter_inset_select(struct i40e_pf *pf,
 		return -EINVAL;
 	}
 
-	pctype = i40e_flowtype_to_pctype(conf->flow_type);
-	if (pctype == 0 || pctype > I40E_FILTER_PCTYPE_L2_PAYLOAD) {
-		PMD_DRV_LOG(ERR, "Not supported flow type (%u)",
-			    conf->flow_type);
+	if (!I40E_VALID_FLOW(conf->flow_type)) {
+		PMD_DRV_LOG(ERR, "invalid flow_type input.");
 		return -EINVAL;
 	}
+	pctype = i40e_flowtype_to_pctype(conf->flow_type);
 	ret = i40e_parse_input_set(&input_set, pctype, conf->field,
 				   conf->inset_size);
 	if (ret) {
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/3] i40e: fix log error
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 1/3] i40e: fix log error Beilei Xing
@ 2016-07-05 13:24     ` Bruce Richardson
  0 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2016-07-05 13:24 UTC (permalink / raw)
  To: Beilei Xing; +Cc: jingjing.wu, michalx.k.jastrzebski, dev

On Tue, Jul 05, 2016 at 02:10:03PM +0800, Beilei Xing wrote:
> The condition, "(pf->flags | I40E_FLAG_VMDQ)" will always be true,
> regardless of the value of the flags operand, because I40E_FLAG_VMDQ
> is 4ULL - meaning at least one bit will always be set in the result.
> That will cause log error when VMDq is disabled.
> Since the original intent behind the condition is to check if VMDq
> is enabled, fix the code by changing "|" to "&".
> 
> Coverity issue: 13219, 13221
> 
> Fixes: 4805ed59e957 ("i40e: enhance mac address operations")
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2 2/3] i40e: fix dereference before null check
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 2/3] i40e: fix dereference before null check Beilei Xing
@ 2016-07-05 13:24     ` Bruce Richardson
  0 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2016-07-05 13:24 UTC (permalink / raw)
  To: Beilei Xing; +Cc: jingjing.wu, michalx.k.jastrzebski, dev

On Tue, Jul 05, 2016 at 02:10:04PM +0800, Beilei Xing wrote:
> Null-checking vsi suggests that it may be null, but it
> has be dereferenced before null-checking. So move if
> statement to the front of assignment statement.
> 
> Coverity: 119265, 119266
> 
> Fixes: d0a349409bd7 ("i40e: support AQ based RSS config")
> Fixes: 647d1eaf758b ("i40evf: support AQ based RSS config")
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access Beilei Xing
@ 2016-07-05 13:26     ` Bruce Richardson
  2016-07-06  2:00       ` Xing, Beilei
  0 siblings, 1 reply; 13+ messages in thread
From: Bruce Richardson @ 2016-07-05 13:26 UTC (permalink / raw)
  To: Beilei Xing; +Cc: jingjing.wu, michalx.k.jastrzebski, dev

On Tue, Jul 05, 2016 at 02:10:05PM +0800, Beilei Xing wrote:
> When calling i40e_flowtype_to_pctype in
> i40e_get_hash_filter_global_config and
> i40e_set_hash_filter_global_config, function
> i40e_flowtype_to_pctype will be possibly
> out-of-bounds accessed, because size of callee's array
> is 15. So judge flow type before calling
> i40e_flowtype_to_pctype.
> Meanwhile do the same change in other functions.
> 
> Coverity issue: 37793, 37794
> 
> Fixes: 782c8c92f13f ("i40e: add hash configuration")
> Fixes: f2b2e2354bbd ("i40e: split function for hash and flow director input")
> Fixes: 98f055707685 ("i40e: configure input fields for RSS or flow director")
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index a1cad37..111a552 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -6908,6 +6908,9 @@ i40e_get_hash_filter_global_config(struct i40e_hw *hw,
>  		mask &= ~(1UL << i);
>  		/* Bit set indicats the coresponding flow type is supported */
>  		g_cfg->valid_bit_mask[0] |= (1UL << i);
> +		/* if flowtype is invalid, continue */
> +		if (!I40E_VALID_FLOW(i))
> +			continue;
>  		pctype = i40e_flowtype_to_pctype(i);
>  		reg = i40e_read_rx_ctl(hw, I40E_GLQF_HSYM(pctype));
>  		if (reg & I40E_GLQF_HSYM_SYMH_ENA_MASK)

Rather than having the same check done in multiple places, is there a reason
why we can't just put the check once in i40e_flowtype_to_pctype?

/Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access
  2016-07-05 13:26     ` Bruce Richardson
@ 2016-07-06  2:00       ` Xing, Beilei
  2016-07-08 15:23         ` Bruce Richardson
  0 siblings, 1 reply; 13+ messages in thread
From: Xing, Beilei @ 2016-07-06  2:00 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: Wu, Jingjing, Jastrzebski, MichalX K, dev



> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, July 5, 2016 9:26 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Jastrzebski, MichalX K
> <michalx.k.jastrzebski@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access
> 
> On Tue, Jul 05, 2016 at 02:10:05PM +0800, Beilei Xing wrote:
> > When calling i40e_flowtype_to_pctype in
> > i40e_get_hash_filter_global_config and
> > i40e_set_hash_filter_global_config, function i40e_flowtype_to_pctype
> > will be possibly out-of-bounds accessed, because size of callee's
> > array is 15. So judge flow type before calling
> > i40e_flowtype_to_pctype.
> > Meanwhile do the same change in other functions.
> >
> > Coverity issue: 37793, 37794
> >
> > Fixes: 782c8c92f13f ("i40e: add hash configuration")
> > Fixes: f2b2e2354bbd ("i40e: split function for hash and flow director
> > input")
> > Fixes: 98f055707685 ("i40e: configure input fields for RSS or flow
> > director")
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index a1cad37..111a552 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -6908,6 +6908,9 @@ i40e_get_hash_filter_global_config(struct
> i40e_hw *hw,
> >  		mask &= ~(1UL << i);
> >  		/* Bit set indicats the coresponding flow type is supported */
> >  		g_cfg->valid_bit_mask[0] |= (1UL << i);
> > +		/* if flowtype is invalid, continue */
> > +		if (!I40E_VALID_FLOW(i))
> > +			continue;
> >  		pctype = i40e_flowtype_to_pctype(i);
> >  		reg = i40e_read_rx_ctl(hw, I40E_GLQF_HSYM(pctype));
> >  		if (reg & I40E_GLQF_HSYM_SYMH_ENA_MASK)
> 
> Rather than having the same check done in multiple places, is there a reason
> why we can't just put the check once in i40e_flowtype_to_pctype?

Since the return value type of i40e_flowtype_to_pctype is " enum i40e_filter_pctype ", although put the check in i40e_flowtype_to_pctype, we should check return value after every i40e_flowtype_to_pctype calling. I think there's no more improvement.
Besides, check valid flow type is called before i40e_flowtype_to_pctype in some places previously, such as function i40e_hash_filter_inset_select and i40e_fdir_filter_inset_select.

/Beilei

> 
> /Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access
  2016-07-06  2:00       ` Xing, Beilei
@ 2016-07-08 15:23         ` Bruce Richardson
  0 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2016-07-08 15:23 UTC (permalink / raw)
  To: Xing, Beilei; +Cc: Wu, Jingjing, Jastrzebski, MichalX K, dev

On Wed, Jul 06, 2016 at 03:00:17AM +0100, Xing, Beilei wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Tuesday, July 5, 2016 9:26 PM
> > To: Xing, Beilei <beilei.xing@intel.com>
> > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Jastrzebski, MichalX K
> > <michalx.k.jastrzebski@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access
> > 
> > On Tue, Jul 05, 2016 at 02:10:05PM +0800, Beilei Xing wrote:
> > > When calling i40e_flowtype_to_pctype in
> > > i40e_get_hash_filter_global_config and
> > > i40e_set_hash_filter_global_config, function i40e_flowtype_to_pctype
> > > will be possibly out-of-bounds accessed, because size of callee's
> > > array is 15. So judge flow type before calling
> > > i40e_flowtype_to_pctype.
> > > Meanwhile do the same change in other functions.
> > >
> > > Coverity issue: 37793, 37794
> > >
> > > Fixes: 782c8c92f13f ("i40e: add hash configuration")
> > > Fixes: f2b2e2354bbd ("i40e: split function for hash and flow director
> > > input")
> > > Fixes: 98f055707685 ("i40e: configure input fields for RSS or flow
> > > director")
> > >
> > > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev.c | 21 ++++++++++++---------
> > >  1 file changed, 12 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index a1cad37..111a552 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -6908,6 +6908,9 @@ i40e_get_hash_filter_global_config(struct
> > i40e_hw *hw,
> > >  		mask &= ~(1UL << i);
> > >  		/* Bit set indicats the coresponding flow type is supported */
> > >  		g_cfg->valid_bit_mask[0] |= (1UL << i);
> > > +		/* if flowtype is invalid, continue */
> > > +		if (!I40E_VALID_FLOW(i))
> > > +			continue;
> > >  		pctype = i40e_flowtype_to_pctype(i);
> > >  		reg = i40e_read_rx_ctl(hw, I40E_GLQF_HSYM(pctype));
> > >  		if (reg & I40E_GLQF_HSYM_SYMH_ENA_MASK)
> > 
> > Rather than having the same check done in multiple places, is there a reason
> > why we can't just put the check once in i40e_flowtype_to_pctype?
> 
> Since the return value type of i40e_flowtype_to_pctype is " enum
> i40e_filter_pctype ", although put the check in i40e_flowtype_to_pctype,
> we should check return value after every i40e_flowtype_to_pctype calling.

Ok, point taken. If we don't check before the call, then the call itself could
return an error which would then need to be checked anyway.

> I think there's no more improvement.
> Besides, check valid flow type is called before i40e_flowtype_to_pctype in
> some places previously, such as function i40e_hash_filter_inset_select
> and i40e_fdir_filter_inset_select.

That's not a reason to keep on putting more checks in other places. It's more
of a reason why the check needs to be centralised. However, from your reasoning
above it doesn't help much, since for these existing cases you'd be just changing
one check for another.

Acked-by: Bruce Richarson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2 0/3] fix coverity defects
  2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
                     ` (2 preceding siblings ...)
  2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access Beilei Xing
@ 2016-07-08 15:33   ` Bruce Richardson
  3 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2016-07-08 15:33 UTC (permalink / raw)
  To: Beilei Xing; +Cc: jingjing.wu, michalx.k.jastrzebski, dev

On Tue, Jul 05, 2016 at 02:10:02PM +0800, Beilei Xing wrote:
> Fix some open coverity defects.
> 
> V2 changes:
>  Rework commit log.
>  Refactor patchset.
> 
> Beilei Xing (3):
>   i40e: fix log error
>   i40e: fix dereference before null check
>   i40e: fix out-of-bounds access
> 
>  drivers/net/i40e/i40e_ethdev.c    | 35 +++++++++++++++++++++--------------
>  drivers/net/i40e/i40e_ethdev_vf.c |  7 +++++--
>  2 files changed, 26 insertions(+), 16 deletions(-)
> 
Applied to dpdk-next-net/rel_16_07

/Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-07-08 15:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30  7:34 [dpdk-dev] [PATCH 0/4] i40e: fix coverity defects Beilei Xing
2016-07-04 13:58 ` Bruce Richardson
2016-07-05  1:47   ` Xing, Beilei
2016-07-05  6:10 ` [dpdk-dev] [PATCH v2 0/3] " Beilei Xing
2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 1/3] i40e: fix log error Beilei Xing
2016-07-05 13:24     ` Bruce Richardson
2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 2/3] i40e: fix dereference before null check Beilei Xing
2016-07-05 13:24     ` Bruce Richardson
2016-07-05  6:10   ` [dpdk-dev] [PATCH v2 3/3] i40e: fix out-of-bounds access Beilei Xing
2016-07-05 13:26     ` Bruce Richardson
2016-07-06  2:00       ` Xing, Beilei
2016-07-08 15:23         ` Bruce Richardson
2016-07-08 15:33   ` [dpdk-dev] [PATCH v2 0/3] fix coverity defects Bruce Richardson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).