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

[Kevin Traynor <ktraynor@redhat.com>]

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 */