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

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

On Wed, Dec 21, 2022 at 05:37:26PM +0100, Alexander Lobakin wrote:
> From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Date: Wed, 21 Dec 2022 10:27:46 +0100
> 
> > KASAN reported:
> > [ 9793.708867] BUG: KASAN: global-out-of-bounds in ice_get_link_speed+0x16/0x30 [ice]
> > [ 9793.709205] Read of size 4 at addr ffffffffc1271b1c by task kworker/6:1/402
> > 
> > [ 9793.709222] CPU: 6 PID: 402 Comm: kworker/6:1 Kdump: loaded Tainted: G    B      OE      6.1.0+ #3
> > [ 9793.709235] Hardware name: Intel Corporation S2600WFT/S2600WFT, BIOS SE5C620.86B.00.01.0014.070920180847 07/09/2018
> > [ 9793.709245] Workqueue: ice ice_service_task [ice]
> 
> [...]
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> > index 4b78bfb0d7f9..a24b5cb95039 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -5562,7 +5562,7 @@ bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw)
> >   * returned by the firmware is a 16 bit * value, but is indexed
> >   * by [fls(speed) - 1]
> >   */
> > -static const u32 ice_aq_to_link_speed[15] = {
> > +static const u32 ice_aq_to_link_speed[] = {
> >  	SPEED_10,	/* BIT(0) */
> >  	SPEED_100,
> >  	SPEED_1000,
> > @@ -5577,7 +5577,8 @@ static const u32 ice_aq_to_link_speed[15] = {
> >  	0,
> >  	0,
> >  	0,
> > -	0		/* BIT(14) */
> > +	0,
> > +	0		/* BIT(15) */
> >  };
> 
> I warned Jesse that no index check might cause out-of-bounds walks
> and here they are. I suggested the following back then:
> 
> 1) Don't define any zeroed elements and elements with
>    %VIRTCHNL_LINK_SPEED_UNKNOWN. Don't define explicit array bounds.
> 
> 2) In ice_get_link_speed():
> 
> 	if (index >= ARRAY_SIZE(ice_aq_to_link_speed))
> 		return 0;
> 
> 	return ice_aq_to_link_speed[index];
> 
> 3) Same in ice_conv_link_speed_to_virtchnl():
> 
> 	u32 index = fls(link_speed) - 1;
> 
> 	if (adv_link_support)
> 		return ice_get_link_speed(index);
> 	else if (index < ARRAY_SIZE(ice_legacy_aq_to_vc_speed))
> 		return ice_legacy_aq_to_vc_speed[index];
> 	else
> 		return VIRTCHNL_LINK_SPEED_UNKNOWN;
> 
> This could go as a fix to net with no problems.
> 

Sounds reasonable, I will do it in next version, thanks.
> >  
> >  /**
> > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_mbx.c b/drivers/net/ethernet/intel/ice/ice_vf_mbx.c
> > index d4a4001b6e5d..5f754d41f345 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_vf_mbx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_vf_mbx.c
> 
> [...]
> 
> > -- 
> > 2.36.1
> 
> Thanks,
> Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan