Re: [Intel-wired-lan] [PATCH net v2] ice: fix out-of-bounds KASAN warning in virtchnl

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Date: Tue, 3 Jan 2023 06:24:01 +0100

> KASAN reported:
> [ 9793.708867] BUG: KASAN: global-out-of-bounds in ice_get_link_speed+0x16/0x30 [ice]
[...]

> The ICE_AQ_LINK_SPEED_UNKNOWN define is BIT(15). The value is bigger
> than both legacy and normal link speed tables. Add one element (0 -
> unknown) to both tables. There is no need to explicitly set table size,
> leave it empty.
> 
> Fixes: 1d0e28a9be1f ("ice: Remove and replace ice speed defines with ethtool.h versions")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
> v1 --> v2:
>  * follow Alex sugestions and remove all zero records from link speed
>    tables and add validation for the index
>  * fix commit message
> ---
> ---

Super-nit: redundant double '---', one is enough.

>  drivers/net/ethernet/intel/ice/ice_common.c |  9 ++++-----
>  drivers/net/ethernet/intel/ice/ice_vf_mbx.c | 18 +++++++-----------
>  2 files changed, 11 insertions(+), 16 deletions(-)

[...]

> @@ -71,21 +67,21 @@ static const u32 ice_legacy_aq_to_vc_speed[15] = {
>   */
>  u32 ice_conv_link_speed_to_virtchnl(bool adv_link_support, u16 link_speed)
>  {
> -	u32 speed;
> +	/* convert a BIT() value into an array index */
> +	u32 index = fls(link_speed) - 1;
>  
>  	if (adv_link_support) {
> -		/* convert a BIT() value into an array index */
> -		speed = ice_get_link_speed(fls(link_speed) - 1);
> -	} else {
> +		return ice_get_link_speed(index);
> +	} else if (index < ARRAY_SIZE(ice_legacy_aq_to_vc_speed)) {
>  		/* Virtchnl speeds are not defined for every speed supported in
>  		 * the hardware. To maintain compatibility with older AVF
>  		 * drivers, while reporting the speed the new speed values are
>  		 * resolved to the closest known virtchnl speeds
>  		 */
> -		speed = ice_legacy_aq_to_vc_speed[fls(link_speed) - 1];
> +		return ice_legacy_aq_to_vc_speed[index];
>  	}

Breh, I've just realized that the branches are now oneliners, so {}s
around them are not needed anymore. Could you please send a v3
without them and with my

Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com>

added?

>  
> -	return speed;
> +	return VIRTCHNL_LINK_SPEED_UNKNOWN;
>  }
>  
>  /* The mailbox overflow detection algorithm helps to check if there
> -- 
> 2.36.1

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan