diff mbox series

[iwl-net,v2] ice: avoid infinite loop if NVM has invalid TLV length

Message ID 20240517-iwl-net-2024-05-16-fix-nvm-tlv-issue-v2-1-fdee184ece86@intel.com
State Superseded
Headers show
Series [iwl-net,v2] ice: avoid infinite loop if NVM has invalid TLV length | expand

Commit Message

Keller, Jacob E May 17, 2024, 11:08 p.m. UTC 
The ice_get_pfa_module_tlv() function iterates over the TLVs in the
Preserved Fields Area (PFA) of the NVM. This is used to access data such as
the Part Board Assembly identifier.

Some NVMs in the wild have been found with incorrect TLV lengths including
at least one which reports a TLV length of 0xFFFF. When trying to read the
PBA from such an NVM, the driver will compute a new offset for the next_tlv
which is lower, due to overflowing the 16-bit next_tlv variable.

In the best case, the driver will incorrectly interpret values until it
finds one which has an offset greater than the PFA area without
overflowing. In the worst case, the values in the NVM result in an infinite
loop as the misinterpreted lengths result in checking offsets which are
valid within the PFA, and which ultimately point in an infinite loop.

Fix this by using check_add_overflow when calculating the NVM offsets, and
bailing if we ever overflow. Additionally, use check_add_overflow when
calculating the initial maximum PFA size.

This ensures that we bail immediately on encountering any TLV who's length
would have caused the naive addition to overflow, rather than entering an
infinite loop or otherwise misinterpreting NVM values.

Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes in v2:
- Use check_add_overflow instead of increasing the variables to u32
- Upgrade log messages to dev_warn()
- Link to v1: https://lore.kernel.org/r/20240516-iwl-net-2024-05-16-fix-nvm-tlv-issue-v1-1-ecbb6a75961e@intel.com
---
 drivers/net/ethernet/intel/ice/ice_nvm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)


---
base-commit: 83e93942796db58652288f0391ac00072401816f
change-id: 20240516-iwl-net-2024-05-16-fix-nvm-tlv-issue-99ebb2c55c52

Best regards,

Comments

Keller, Jacob E May 17, 2024, 11:10 p.m. UTC | #1 
On 5/17/2024 4:08 PM, Jacob Keller wrote:
> The ice_get_pfa_module_tlv() function iterates over the TLVs in the
> Preserved Fields Area (PFA) of the NVM. This is used to access data such as
> the Part Board Assembly identifier.
> 
> Some NVMs in the wild have been found with incorrect TLV lengths including
> at least one which reports a TLV length of 0xFFFF. When trying to read the
> PBA from such an NVM, the driver will compute a new offset for the next_tlv
> which is lower, due to overflowing the 16-bit next_tlv variable.
> 
> In the best case, the driver will incorrectly interpret values until it
> finds one which has an offset greater than the PFA area without
> overflowing. In the worst case, the values in the NVM result in an infinite
> loop as the misinterpreted lengths result in checking offsets which are
> valid within the PFA, and which ultimately point in an infinite loop.
> 
> Fix this by using check_add_overflow when calculating the NVM offsets, and
> bailing if we ever overflow. Additionally, use check_add_overflow when
> calculating the initial maximum PFA size.
> 
> This ensures that we bail immediately on encountering any TLV who's length
> would have caused the naive addition to overflow, rather than entering an
> infinite loop or otherwise misinterpreting NVM values.
> 
> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Fixes: e961b679fb0b ("ice: add board identifier info to devlink .info_get")

I'll ensure this gets added to the commit when queuing, as I forgot to
add it when re-writing this commit message.
Keller, Jacob E May 17, 2024, 11:18 p.m. UTC | #2 
On 5/17/2024 4:08 PM, Jacob Keller wrote:

> -		next_tlv = next_tlv + tlv_len + 2;
> +		if (check_add_overflow(next_tlv, 2, &next_tlv) ||
> +		    check_add_overflow(next_tlv, tlv_len, &next_tlv))
> +			dev_warn(ice_hw_to_dev(hw), "Failed to locate PFA TLV module of type %u due to arithmetic overflow. The PFA length is %u. The last TLV has length of %u\n",
> +				 module_type, pfa_len, tlv_len);
> +			return -EINVAL;
> +		}

This hunk is missing a { which I had fixed locally but forgot to ammend.
I'll have it fixed in a v3 later.. :(

Thanks,
Jake
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 84eab92dc03c..53dbc9000652 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -440,8 +440,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;
-	u16 next_tlv;
+	u16 pfa_len, pfa_ptr, next_tlv, max_pfa;
 	int status;
 
 	status = ice_read_sr_word(hw, ICE_SR_PFA_PTR, &pfa_ptr);
@@ -454,11 +453,18 @@  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, pfa_len, &max_pfa)) {
+		dev_warn(ice_hw_to_dev(hw), "PFA starts at offset %u. PFA length of %u causes 16-bit arithmetic overflow.\n",
+			 pfa_len);
+		return -EINVAL;
+	}
+
 	/* 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 < pfa_ptr + pfa_len) {
+	while (next_tlv < max_pfa) {
 		u16 tlv_sub_module_type;
 		u16 tlv_len;
 
@@ -485,7 +491,12 @@  ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
 		/* 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, 2, &next_tlv) ||
+		    check_add_overflow(next_tlv, tlv_len, &next_tlv))
+			dev_warn(ice_hw_to_dev(hw), "Failed to locate PFA TLV module of type %u due to arithmetic overflow. The PFA length is %u. The last TLV has length of %u\n",
+				 module_type, pfa_len, tlv_len);
+			return -EINVAL;
+		}
 	}
 	/* Module does not exist */
 	return -ENOENT;