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

[Keller, Jacob E]

> -----Original Message-----
> From: Paul Menzel <pmenzel at molgen.mpg.de>
> Sent: Thursday, May 16, 2024 11:52 PM
> To: Keller, Jacob E <jacob.e.keller at intel.com>
> Cc: Greenwalt, Paul <paul.greenwalt at intel.com>; intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix integer overflow in
> ice_get_pfa_module_tlv
> 
> 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?
> 

The TLV size is 16bit, so we'd need to have more than 2^16 TLVs to overflow a 32 bit length. In particular the NVMs which this fixes were reporting an incorrect TLV length of 0xFFFF, which caused us to overflow the size. This resulted in the driver incorrectly looping over and over in such a way that it never exited the TLV loop. This is a buggy NVM, and the goal here is to simply prevent the driver from infinitely looping, as this causes devlink dev info to lockup and prevents driver removal or any other form of recovery.

I suppose an alternative fix would be to use something like the overflow.h helpers which saturate instead of overflowing, but those are all size_t based.

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

Its an offset into the Shadow RAM. The terminology used by both the datasheet and the NVM documentation is a pointer: we read from the Shadow RAM  the ICE_SR_PFA_PTR, which points us to where the PFA starts. Its not a pointer in the kernel or C technical sense.

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

I can change this. Its unexpected as it is caused by a misconfigured NVM. Strictly speaking I think this check itself is sufficient since the PFA length cant be greater than 16bits. Changing the other values to 32 bit ensures that those don't overflow and incorrectly compare true as well. The original fix was developed by Paul Greenwalt, so he might have some context I don't.

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