diff mbox series

[v1,08/12] net/ice: update iteration of TLVs in Preserved Fields Area

Message ID 20240822095612.216214-9-soumyadeep.hore@intel.com (mailing list archive)
State Superseded
Delegated to: Bruce Richardson
Headers
Series Align ICE shared code with Base driver |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Hore, Soumyadeep Aug. 22, 2024, 9:56 a.m. UTC 
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: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/ice/base/ice_nvm.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Comments

Bruce Richardson Aug. 22, 2024, 2:45 p.m. UTC | #1 
On Thu, Aug 22, 2024 at 09:56:08AM +0000, Soumyadeep Hore wrote:
> 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: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
Is this related to patch 4 of this set? Should they be merged or are they
solving different issues?
Hore, Soumyadeep Aug. 23, 2024, 6:55 a.m. UTC | #2 
Hi Bruce, they should not be merged, they are solving different issues.

On Thu, Aug 22, 2024 at 09:56:08AM +0000, Soumyadeep Hore wrote:
> 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: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
Is this related to patch 4 of this set? Should they be merged or are they solving different issues?
diff mbox series

Patch

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) {
 		u16 tlv_sub_module_type;
 		u16 tlv_len;
 
@@ -524,10 +532,6 @@  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 TLV length.\n");
 			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) {
 				*module_tlv = (u16)next_tlv;
@@ -536,10 +540,13 @@  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 */
 	return ICE_ERR_DOES_NOT_EXIST;