[v3,08/12] net/ice: update iteration of TLVs in Preserved Fields Area
Checks
Commit Message
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(-)
Comments
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
@@ -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;