On Fri, Aug 23, 2024 at 09:56:46AM +0000, Soumyadeep Hore wrote:
> From: Fabio Pricoco <fabio.pric...@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.pric...@intel.com>
> Signed-off-by: Soumyadeep Hore <soumyadeep.h...@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
