Re: [Intel-wired-lan] [PATCH iwl-net v3] ice: avoid infinite loop if NVM has invalid TLV length

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

On 5/18/24 01:22, 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.
> 
> 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 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 | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index 84eab92dc03c..be731b83d667 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_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_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;
> 
> ---
> base-commit: 83e93942796db58652288f0391ac00072401816f
> change-id: 20240516-iwl-net-2024-05-16-fix-nvm-tlv-issue-99ebb2c55c52
> 
> Best regards,

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>