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>]

When would you want this done by? I'd like to give any patch a quick test before I say it's good to go.

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, December 3, 2024 3:37 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Pricoco, Fabio
> <fabio.pricoco@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 03/12/2024 16:10, Richardson, Bruce wrote:
> > 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);
> > }
> >
> 
> LGTM and better than my version because it will minimise the changes
> from the main branch commit.
> 
> Considering its fairly trivial but not tested, do you think it warrants
> adding to 21.11 LTS ?
> 
> If so, quickest would be for me to just rebase the existing patch with
> above code and (with your permission) add your sign-off, or if you
> prefer you could send the rebased patch.
> 
> >> -----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 */
> >