[Intel-wired-lan] [PATCH iwl-net] ice: fix integer overflow in ice_get_pfa_module_tlv

[Paul Menzel]

Dear Paul, dear Jacob,


Am 16.05.24 um 23:18 schrieb Jacob Keller:
> From: Paul Greenwalt <paul.greenwalt at intel.com>
> 
> It's possible that an NVM with an invalid tlv_len could cause an integer
> overflow of next_tlv which can result an infinite loop.
> 
> Fix this issue by changing next_tlv from u16 to u32 to prevent overflow.

Why is 32-bit enough?

> Also check that tlv_len is valid and less than pfa_len.
> 
> Signed-off-by: Paul Greenwalt <paul.greenwalt at intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_nvm.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index 84eab92dc03c..9e58d319355f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> @@ -441,7 +441,7 @@ ice_get_pfa_module_tlv(struct ice_hw *hw, u16 *module_tlv, u16 *module_tlv_len,
>   		       u16 module_type)
>   {
>   	u16 pfa_len, pfa_ptr;

By the way, is pfa_ptr an actual pointer address?

> -	u16 next_tlv;
> +	u32 next_tlv;
>   	int status;
>   
>   	status = ice_read_sr_word(hw, ICE_SR_PFA_PTR, &pfa_ptr);
> @@ -458,7 +458,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 < pfa_ptr + pfa_len) {
> +	while (next_tlv < ((u32)pfa_ptr + pfa_len)) {
>   		u16 tlv_sub_module_type;
>   		u16 tlv_len;
>   
> @@ -474,6 +474,10 @@ 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");

Please print both values. Should this be at least a warning, if it’s not 
an expected situation?

> +			return -EINVAL;
> +		}
>   		if (tlv_sub_module_type == module_type) {
>   			if (tlv_len) {
>   				*module_tlv = next_tlv;


Kind regards,

Paul