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 04:37:00PM +0100, Paul Menzel wrote:
> [CC: +Brett, +Alexander, +Jesse, +Gurucharan]
> 
> Dear Michal,
> 
> 
> Thank you for your patch.
> 
> 
> Am 21.12.22 um 10:27 schrieb Michal Swiatkowski:
> 
> In the summary, I normally prefer to not the action. (There is also a typo
> in *warning*.
> 
> > 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]
> > [ 9793.709575] Call Trace:
> > [ 9793.709582]  <TASK>
> > [ 9793.709588]  dump_stack_lvl+0x44/0x5c
> > [ 9793.709613]  print_report+0x17f/0x47b
> > [ 9793.709632]  ? __cpuidle_text_end+0x5/0x5
> > [ 9793.709653]  ? ice_get_link_speed+0x16/0x30 [ice]
> > [ 9793.709986]  ? ice_get_link_speed+0x16/0x30 [ice]
> > [ 9793.710317]  kasan_report+0xb7/0x140
> > [ 9793.710335]  ? ice_get_link_speed+0x16/0x30 [ice]
> > [ 9793.710673]  ice_get_link_speed+0x16/0x30 [ice]
> > [ 9793.711006]  ice_vc_notify_vf_link_state+0x14c/0x160 [ice]
> > [ 9793.711351]  ? ice_vc_repr_cfg_promiscuous_mode+0x120/0x120 [ice]
> > [ 9793.711698]  ice_vc_process_vf_msg+0x7a7/0xc00 [ice]
> > [ 9793.712074]  __ice_clean_ctrlq+0x98f/0xd20 [ice]
> > [ 9793.712534]  ? ice_bridge_setlink+0x410/0x410 [ice]
> > [ 9793.712979]  ? __request_module+0x320/0x520
> > [ 9793.713014]  ? ice_process_vflr_event+0x27/0x130 [ice]
> > [ 9793.713489]  ice_service_task+0x11cf/0x1950 [ice]
> > [ 9793.713948]  ? io_schedule_timeout+0xb0/0xb0
> > [ 9793.713972]  process_one_work+0x3d0/0x6a0
> > [ 9793.714003]  worker_thread+0x8a/0x610
> > [ 9793.714031]  ? process_one_work+0x6a0/0x6a0
> > [ 9793.714049]  kthread+0x164/0x1a0
> > [ 9793.714071]  ? kthread_complete_and_exit+0x20/0x20
> > [ 9793.714100]  ret_from_fork+0x1f/0x30
> > [ 9793.714137]  </TASK>
> > 
> > [ 9793.714151] The buggy address belongs to the variable:
> > [ 9793.714158]  ice_aq_to_link_speed+0x3c/0xffffffffffff3520 [ice]
> > 
> > [ 9793.714632] Memory state around the buggy address:
> > [ 9793.714642]  ffffffffc1271a00: f9 f9 f9 f9 00 00 05 f9 f9 f9 f9 f9 00 00 02 f9
> > [ 9793.714656]  ffffffffc1271a80: f9 f9 f9 f9 00 00 04 f9 f9 f9 f9 f9 00 00 00 00
> > [ 9793.714670] >ffffffffc1271b00: 00 00 00 04 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
> > [ 9793.714680]                             ^
> > [ 9793.714690]  ffffffffc1271b80: 00 00 00 00 00 04 f9 f9 f9 f9 f9 f9 00 00 00 00
> > [ 9793.714704]  ffffffffc1271c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> Do you need to do something special to trigger this? I am curious, why this
> wasn’t found, when it has a `Tested-by` tag.
> 

Only create VFs ex.
# echo 4 > /sys/bus/pci/devices/0000\:18\:00.0/sriov_numvfs

I don't know why it wasn't found, I was checking sth else with KASAN and
saw it.

> > 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 explicit set table size,
> 
> explicit*ly*?
>
Yeah, thanks

> > leave it empty.
> > 
> > Fixes: 1d0e28a9be1f ("ice: Remove and replace ice speed defines with ethtool.h versions")
> 
> This is in next-20221202. (Can commits in linux-next be amended? Probably
> not.)
> 
Maybe I copied it wrong, will fix.

> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_common.c | 5 +++--
> >   drivers/net/ethernet/intel/ice/ice_vf_mbx.c | 5 +++--
> >   2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > 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) */
> >   };
> >   /**
> > 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
> > @@ -39,7 +39,7 @@ ice_aq_send_msg_to_vf(struct ice_hw *hw, u16 vfid, u32 v_opcode, u32 v_retval,
> >   	return ice_sq_send_cmd(hw, &hw->mailboxq, &desc, msg, msglen, cd);
> >   }
> > -static const u32 ice_legacy_aq_to_vc_speed[15] = {
> > +static const u32 ice_legacy_aq_to_vc_speed[] = {
> >   	VIRTCHNL_LINK_SPEED_100MB,	/* BIT(0) */
> >   	VIRTCHNL_LINK_SPEED_100MB,
> >   	VIRTCHNL_LINK_SPEED_1GB,
> > @@ -54,7 +54,8 @@ static const u32 ice_legacy_aq_to_vc_speed[15] = {
> >   	VIRTCHNL_LINK_SPEED_UNKNOWN,
> >   	VIRTCHNL_LINK_SPEED_UNKNOWN,
> >   	VIRTCHNL_LINK_SPEED_UNKNOWN,
> > -	VIRTCHNL_LINK_SPEED_UNKNOWN	/* BIT(14) */
> > +	VIRTCHNL_LINK_SPEED_UNKNOWN,
> > +	VIRTCHNL_LINK_SPEED_UNKNOWN	/* BIT(15) */
> >   };
> >   /**
> 
> The rest looks good.

Thanks for Your review
> 
> Kind regards,
> 
> Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan