diff mbox

[dpdk-dev,v2,3/3] i40e: fix out-of-bounds access

Message ID 1467699005-16235-4-git-send-email-beilei.xing@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Bruce Richardson
Headers

Commit Message

Xing, Beilei July 5, 2016, 6:10 a.m. UTC 
  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(-)

Comments

Bruce Richardson July 5, 2016, 1:26 p.m. UTC | #1 
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
Xing, Beilei July 6, 2016, 2 a.m. UTC | #2 
> -----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
Bruce Richardson July 8, 2016, 3:23 p.m. UTC | #3 
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>
diff mbox

Patch

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) {