RE: patch 'net/ice/base: fix iteration of TLVs in Preserved Fields Area' has been queued to stable release 21.11.9

["Richardson, Bruce" <bruce.richardson@intel.com>]

Rather than trying to rework the loop code, I would think it would be better to provide a new local definition of "check_add_overflow", since it's used 3 times in the patch, not just once. The implementation only needs to support u16 values. Something like (again not tested):

bool
check_add_overflow(u16 a, u16 b, u16 *out)
{
	u32 val = (u32)a + (u32)b;
	*out = (uint16_t)val;

              return (val > UINT16_MAX);
}

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, December 3, 2024 2:13 PM
> To: Pricoco, Fabio <fabio.pricoco@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Hore, Soumyadeep
> <soumyadeep.hore@intel.com>; dpdk stable <stable@dpdk.org>
> Subject: Re: patch 'net/ice/base: fix iteration of TLVs in Preserved Fields Area'
> has been queued to stable release 21.11.9
> 
> On 27/11/2024 18:17, Kevin Traynor wrote:
> > Hi,
> >
> > FYI, your patch has been queued to stable release 21.11.9
> >
> 
> Hi Bruce/Fabio,
> 
> This patch is causing a build issue for CentOS7 as
> __builtin_add_overflow is not available on GCC 4.8.5.
> 
> Though CentOS7 has just gone EoL, I don't want to break the build with
> the last 21.11 LTS release.
> 
> I think we could replace the check_add_overflow/builtin with something
> like below (untested), but if the patch isn't important for 21.11
> branch, then it's probably safer to drop it.
> 
> How much is the patch needed for 21.11 branch ?
> 
> thanks,
> Kevin.
> 
> --- a/drivers/net/ice/base/ice_nvm.c
> +++ b/drivers/net/ice/base/ice_nvm.c
> @@ -427,6 +427,4 @@ enum ice_status ice_read_sr_word(struct ice_hw
> *hw,
> u16 offset, u16 *data)
>  }
> 
> -#define check_add_overflow __builtin_add_overflow
> -
>  /**
>   * ice_get_pfa_module_tlv - Reads sub module TLV from NVM PFA
> @@ -458,5 +456,6 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
>         }
> 
> -       if (check_add_overflow(pfa_ptr, (u16)(pfa_len - 1), &max_tlv)) {
> +       max_tlv = pfa_ptr + pfa_len - 1;
> +       if (pfa_ptr > max_tlv) {
>                 ice_debug(hw, ICE_DBG_INIT, "PFA starts at offset %u.
> PFA length of %u caused 16-bit arithmetic overflow.\n",
>                                   pfa_ptr, pfa_len);
> @@ -475,4 +474,5 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
>         while (next_tlv < max_tlv) {
>                 u16 tlv_sub_module_type;
> +               u16 curr_tlv;
>                 u16 tlv_len;
> 
> @@ -499,6 +499,7 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
>                 }
> 
> -               if (check_add_overflow(next_tlv, (u16)2, &next_tlv) ||
> -                   check_add_overflow(next_tlv, tlv_len, &next_tlv)) {
> +               curr_tlv = next_tlv;
> +               next_tlv = next_tlv + tlv_len + 2;
> +               if (curr_tlv > next_tlv) {
>                         ice_debug(hw, ICE_DBG_INIT, "TLV of type %u and
> length 0x%04x caused 16-bit arithmetic overflow. The PFA starts at
> 0x%04x and has length of 0x%04x\n",
>                                           tlv_sub_module_type, tlv_len,
> pfa_ptr, pfa_len);
> 
> 
> > Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
> > It will be pushed if I get no objections before 12/02/24. So please
> > shout if anyone has objections.
> >
> > Also note that after the patch there's a diff of the upstream commit vs the
> > patch applied to the branch. This will indicate if there was any rebasing
> > needed to apply to the stable branch. If there were code changes for
> rebasing
> > (ie: not only metadata diffs), please double check that the rebase was
> > correctly done.
> >
> > Queued patches are on a temporary branch at:
> > https://github.com/kevintraynor/dpdk-stable
> >
> > This queued commit can be viewed at:
> > https://github.com/kevintraynor/dpdk-
> stable/commit/ebbecb19ccdb0df88227e69b2a83ae2ee79a2d19
> >
> > Thanks.
> >
> > Kevin
> >
> > ---
> > From ebbecb19ccdb0df88227e69b2a83ae2ee79a2d19 Mon Sep 17
> 00:00:00 2001
> > From: Fabio Pricoco <fabio.pricoco@intel.com>
> > Date: Fri, 23 Aug 2024 09:56:42 +0000
> > Subject: [PATCH] net/ice/base: fix iteration of TLVs in Preserved Fields Area
> >
> > [ upstream commit dcb760bf0f951b404bce33a1dd14906154b58c75 ]
> >
> > The ice_get_pfa_module_tlv() function iterates over the Preserved Fields
> > Area to read data from the Shadow RAM, including the Part Board Assembly
> > data, among others.
> >
> > If the specific TLV being requested is not found in the current NVM, the
> > code will read past the end of the PFA, misinterpreting the last word of
> > the PFA and the word just after the PFA as another TLV. This typically
> > results in one extra iteration before the length check of the while loop
> > is triggered.
> >
> > Correct the logic for determining the maximum PFA offset to include the
> > extra last word. Additionally, make the driver robust against overflows
> > by using check_add_overflow. This ensures that even if the NVM provides
> > bogus data, the driver will not overflow, and will instead log a useful
> > warning message. The check for whether the TLV length exceeds the PFA
> > length is also removed, in favor of relying on the overflow warning
> > instead.
> >
> > Fixes: 5d0b7b5fc491 ("net/ice/base: add read PBA module function")
> >
> > Signed-off-by: Fabio Pricoco <fabio.pricoco@intel.com>
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/net/ice/base/ice_nvm.c | 36 ++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ice/base/ice_nvm.c b/drivers/net/ice/base/ice_nvm.c
> > index 48e0d418e2..c5a3eddebf 100644
> > --- a/drivers/net/ice/base/ice_nvm.c
> > +++ b/drivers/net/ice/base/ice_nvm.c
> > @@ -427,4 +427,6 @@ enum ice_status ice_read_sr_word(struct ice_hw
> *hw, u16 offset, u16 *data)
> >  }
> >
> > +#define check_add_overflow __builtin_add_overflow
> > +
> >  /**
> >   * ice_get_pfa_module_tlv - Reads sub module TLV from NVM PFA
> > @@ -443,6 +445,5 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
> >  {
> >  	enum ice_status status;
> > -	u16 pfa_len, pfa_ptr;
> > -	u32 next_tlv;
> > +	u16 pfa_len, pfa_ptr, next_tlv, max_tlv;
> >
> >  	status = ice_read_sr_word(hw, ICE_SR_PFA_PTR, &pfa_ptr);
> > @@ -456,9 +457,21 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
> >  		return status;
> >  	}
> > -	/* Starting with first TLV after PFA length, iterate through the list
> > +
> > +	if (check_add_overflow(pfa_ptr, (u16)(pfa_len - 1), &max_tlv)) {
> > +		ice_debug(hw, ICE_DBG_INIT, "PFA starts at offset %u. PFA
> length of %u caused 16-bit arithmetic overflow.\n",
> > +				  pfa_ptr, pfa_len);
> > +		return ICE_ERR_INVAL_SIZE;
> > +	}
> > +
> > +	/* The Preserved Fields Area contains a sequence of TLVs which define
> > +	 * its contents. The PFA length includes all of the TLVs, plus its
> > +	 * initial length word itself, *and* one final word at the end of all
> > +	 * of the TLVs.
> > +	 *
> > +	 * Starting with first TLV after PFA length, iterate through the list
> >  	 * of TLVs to find the requested one.
> >  	 */
> >  	next_tlv = pfa_ptr + 1;
> > -	while (next_tlv < ((u32)pfa_ptr + pfa_len)) {
> > +	while (next_tlv < max_tlv) {
> >  		u16 tlv_sub_module_type;
> >  		u16 tlv_len;
> > @@ -477,8 +490,4 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
> >  			break;
> >  		}
> > -		if (tlv_len > pfa_len) {
> > -			ice_debug(hw, ICE_DBG_INIT, "Invalid TLV length.\n");
> > -			return ICE_ERR_INVAL_SIZE;
> > -		}
> >  		if (tlv_sub_module_type == module_type) {
> >  			if (tlv_len) {
> > @@ -489,8 +498,11 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16
> *module_tlv, u16 *module_tlv_len,
> >  			return ICE_ERR_INVAL_SIZE;
> >  		}
> > -		/* Check next TLV, i.e. current TLV pointer + length + 2 words
> > -		 * (for current TLV's type and length)
> > -		 */
> > -		next_tlv = next_tlv + tlv_len + 2;
> > +
> > +		if (check_add_overflow(next_tlv, (u16)2, &next_tlv) ||
> > +		    check_add_overflow(next_tlv, tlv_len, &next_tlv)) {
> > +			ice_debug(hw, ICE_DBG_INIT, "TLV of type %u and
> length 0x%04x caused 16-bit arithmetic overflow. The PFA starts at 0x%04x
> and has length of 0x%04x\n",
> > +					  tlv_sub_module_type, tlv_len,
> pfa_ptr, pfa_len);
> > +			return ICE_ERR_INVAL_SIZE;
> > +		}
> >  	}
> >  	/* Module does not exist */