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

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Tue, Jan 03, 2023 at 02:26:37PM +0100, Alexander Lobakin wrote:
> 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?
> 
Sure, thanks for review. I will fix it and also remove double "---".

> >  
> > -	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