Re: [PATCH v3 08/12] net/ice: update iteration of TLVs in Preserved Fields Area

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

On Fri, Aug 23, 2024 at 09:56:46AM +0000, Soumyadeep Hore wrote:
> From: Fabio Pricoco <fabio.pricoco@intel.com>
> 
> 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.
> 
> Signed-off-by: Fabio Pricoco <fabio.pricoco@intel.com>
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/net/ice/base/ice_nvm.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ice/base/ice_nvm.c b/drivers/net/ice/base/ice_nvm.c
> index 0124cef04c..56c6c96a95 100644
> --- a/drivers/net/ice/base/ice_nvm.c
> +++ b/drivers/net/ice/base/ice_nvm.c
> @@ -469,6 +469,8 @@ int ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data)
>  	return status;
>  }
>  
> +#define check_add_overflow __builtin_add_overflow
> +
>  /**
>   * ice_get_pfa_module_tlv - Reads sub module TLV from NVM PFA
>   * @hw: pointer to hardware structure
> @@ -484,8 +486,7 @@ int
>  ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
>  		       u16 module_type)
>  {
> -	u16 pfa_len, pfa_ptr;
> -	u32 next_tlv;
> +	u16 pfa_len, pfa_ptr, next_tlv, max_tlv;
>  	int status;
>  
>  	status = ice_read_sr_word(hw, ICE_SR_PFA_PTR, &pfa_ptr);
> @@ -498,6 +499,13 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
>  		ice_debug(hw, ICE_DBG_INIT, "Failed to read PFA length.\n");
>  		return status;
>  	}
> +
> +	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
> @@ -507,7 +515,7 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
>  	 * of TLVs to find the requested one.
>  	 */
>  	next_tlv = pfa_ptr + 1;
> -	while (next_tlv < ((u32)pfa_ptr + pfa_len - 1)) {
> +	while (next_tlv < max_tlv) {

This is essentially overwriting the change made in patch 4 of this set -
except for the comment change. Therefore, I believe patches 4 and 8 should
be merged to avoid touching this line of code multiple times.

/Bruce