Re: [Intel-wired-lan] [PATCH iwl-net v4] ice: fix iteration of TLVs in Preserved Fields Area

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 5/25/24 01:06, Jacob Keller wrote:
> The ice_get_pfa_module_tlv() function iterates over the Type-Length-Value
> structures in the Preserved Fields Area (PFA) of the NVM. This is used by
> the driver to access data such as the Part Board Assembly identifier.
> 
> The function uses simple logic to iterate over the PFA. First, the pointer
> to the PFA in the NVM is read. Then the total length of the PFA is read
> from the first word.
> 
> A pointer to the first TLV is initialized, and a simple loop iterates over
> each TLV. The pointer is moved forward through the NVM until it exceeds the
> PFA area.
> 
> The logic seems sound, but it is missing a key detail. The Preserved
> Fields Area length includes one additional final word. This is documented
> in the device data sheet as a dummy word which contains 0xFFFF. All NVMs
> have this extra word.
> 
> If the driver tries to scan for a TLV that is not in the PFA, it will read
> past the size of the PFA. It reads and interprets the last dummy word of
> the PFA as a TLV with type 0xFFFF. It then reads the word following the PFA
> as a length.
> 
> The PFA resides within the Shadow RAM portion of the NVM, which is
> relatively small. All of its offsets are within a 16-bit size. The PFA
> pointer and TLV pointer are stored by the driver as 16-bit values.
> 
> In almost all cases, the word following the PFA will be such that
> interpreting it as a length will result in 16-bit arithmetic overflow. Once
> overflowed, the new next_tlv value is now below the maximum offset of the
> PFA. Thus, the driver will continue to iterate the data as TLVs. In the
> worst case, the driver hits on a sequence of reads which loop back to
> reading the same offsets in an endless loop.
> 
> To fix this, we need to correct the loop iteration check to account for
> this extra word at the end of the PFA. This alone is sufficient to resolve
> the known cases of this issue in the field. However, it is plausible that
> an NVM could be misconfigured or have corrupt data which results in the
> same kind of overflow. Protect against this by using check_add_overflow
> when calculating both the maximum offset of the TLVs, and when calculating
> the next_tlv offset at the end of each loop iteration. This ensures that
> the driver will not get stuck in an infinite loop when scanning the PFA.
> 
> Fixes: e961b679fb0b ("ice: add board identifier info to devlink .info_get")
> 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 v4:
> - Update title and description for true root cause

Thank you for the fix and thoughtful description of the issue,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> - Correct driver logic to account for PFA length being 1 larger than TLVs
> - Link to v3: https://lore.kernel.org/r/20240517-iwl-net-2024-05-16-fix-nvm-tlv-issue-v3-1-f46c53cfb67f@intel.com
> 
> Changes in v3:
> - Fix missing {
> - Fix missing pfa_ptr variable to dev_warn()
> - Add Fixes tag
> - Link to v2: https://lore.kernel.org/r/20240517-iwl-net-2024-05-16-fix-nvm-tlv-issue-v2-1-fdee184ece86@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 | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index 84eab92dc03c..ea7cbdf8492d 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_tlv;
>   	int status;
>   
>   	status = ice_read_sr_word(hw, ICE_SR_PFA_PTR, &pfa_ptr);
> @@ -454,11 +453,23 @@ 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;
>   	}
> +
> +	/* The Preserved Fields Area contains a sequence of Type-Length-Value
> +	 * structures which define its contents. The PFA length includes all
> +	 * of the TLVs, plus the initial length word itself, *and* one final
> +	 * word at the end after all of the TLVs.
> +	 */
> +	if (check_add_overflow(pfa_ptr, pfa_len - 1, &max_tlv)) {
> +		dev_warn(ice_hw_to_dev(hw), "PFA starts at offset %u. PFA length of %u caused 16-bit arithmetic overflow.\n",
> +			 pfa_ptr, 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_tlv) {
>   		u16 tlv_sub_module_type;
>   		u16 tlv_len;
>   
> @@ -482,10 +493,13 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
>   			}
>   			return -EINVAL;
>   		}
> -		/* 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), "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 -EINVAL;
> +		}
>   	}
>   	/* Module does not exist */
>   	return -ENOENT;
> 
> ---
> base-commit: 83e93942796db58652288f0391ac00072401816f
> change-id: 20240516-iwl-net-2024-05-16-fix-nvm-tlv-issue-99ebb2c55c52
> 
> Best regards,