Re: [Intel-wired-lan] [PATCH v2] e100: prevent shift-out-of-bounds in e100_eeprom_load()

[Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>]

On Mon, Aug 10, 2026 at 08:49:42AM +0000, Loktionov, Aleksandr wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> > Of pavankumaryalagada@gmail.com
> > Sent: Monday, August 10, 2026 10:34 AM
> > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> > Przemyslaw <przemyslaw.kitszel@intel.com>
> > Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> > kuba@kernel.org; pabeni@redhat.com; intel-wired-lan@lists.osuosl.org;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > skhan@linuxfoundation.org;
> > syzbot+e0abb1d45ac291ebebeb@syzkaller.appspotmail.com; Yalagada Pavan
> > Kumar <pavankumaryalagada@gmail.com>
> > Subject: [Intel-wired-lan] [PATCH v2] e100: prevent shift-out-of-
> > bounds in e100_eeprom_load()
> > 
> > From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > 
> > When reading the EEPROM address length, e100_eeprom_read() can return
> > an invalid length (0 or >= 16). Passing an invalid addr_len to bit-
> > shift operations causes a shift out-of-bounds, triggering a kernel
> > panic or UBSAN warning.
> > 
> > Validate addr_len after reading it from EEPROM in both
> > e100_eeprom_load() and e100_eeprom_save(), and return -EIO if the
> > value is out of bounds.
> > 
> > Reported-by: syzbot+e0abb1d45ac291ebebeb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=e0abb1d45ac291ebebeb
> > Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > ---
> > Tested in QEMU using syzbot c reproducer.
> > 
> > v2:
> > - Return -EIO instead of -EINVAL for an invalid EEPROM address length.
> > - Validate addr_len in e100_eeprom_save() as well.
> > - Drop the unnecessary 1U change in the shift.
> > - Update the commit message.
> > 
> > v1:
> > https://lore.kernel.org/all/20260807145626.52692-1-
> > pavankumaryalagada@gmail.com/T/
> > ---
> >  drivers/net/ethernet/intel/e100.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/e100.c
> > b/drivers/net/ethernet/intel/e100.c
> > index 1de5cd41ea0c..464dc2d6cdb5 100644
> > --- a/drivers/net/ethernet/intel/e100.c
> > +++ b/drivers/net/ethernet/intel/e100.c
> > @@ -775,10 +775,10 @@ static int e100_eeprom_load(struct nic *nic)
> >  		netif_err(nic, probe, nic->netdev,
> >  			"Invalid EEPROM address length %u\n",
> >  			addr_len);
> > -		return -EINVAL;
> > +		return -EIO;
> >  	}
> > 
> > -	nic->eeprom_wc = 1U << addr_len;
> > +	nic->eeprom_wc = 1 << addr_len;
> Why do you drop U suffix? For me it looks like you trade one warning for another.

I dropped the `U` suffix in v2 based on previous review. My understanding from that
review was that the `U` suffix is not what prevents the shift-out-of-bounds issue.

The problem is that `addr_len` can underflow as a `u16` and become a large value
such as 65529.

In that case, both `1 << addr_len` and `1U << addr_len` would have an invalid shift count.
Validating addr_len before calculating `eeprom_wc` is what prevents the invalid shift.

The reason i used `1U << addr_len` in v1 was to make the left operand unsigned.

However, after validating `addr_len` to supported range, the maximum shift is 8, so the U
suffix is not needed to prevent signed overflow.

I also tested the reproducer with all three forms:
	1U << addr_len
	1 << addr_len
	(u16)BIT(addr_len)

They all produced the same result with reproducer because the invalid address length is
rejected before the shift is performed.

Regarding the validation range, I initially used
	if (!addr_len || addr_len >= 16)

because the reported value was 65529 and this was sufficient to reject
it before the shift.

However, after reviewing the Intel 8255x Software Developer Manual,
i found that the EEPROM address field is 6 bits for a 64-regsiter EEPROM
and 8 bits for a 256-register EEPROM.

the driver also has:
	__le16 eeprom[256];

and calculates the EEPROM word count from address length.

Therefore, i think if (!addr_len || addr_len > 8) is more
appropriate validation than `>= 16`.
it validates the actual supported EEPROM address length.

> I'm for explicit (u16)BIT(addr_len), what do you think?

for the calculation itself, i agree with using:
	nic->eeprom_wc = (u16)BIT(addr_len);

1 << addr_len means shifting the value 1 by addr_len bits

for example: 
an 8-bit EEPROM address length gives 1 << 8 or 256 EEPROM words.

BIT(addr_len) expresses this bit operation explicitly, and the
(u16) cast makes result type match nic->eeprom_wc.

I will therefore change both e100_eeprom_load() and e100_eeprom_save()
to validate with addr_len > 8 and use (u16)BIT(addr_len).

Please let me know if you agree with using addr_len > 8 based on
the EEPROM address length limitation.

Also, would you prefer (u16)BIT(addr_len) for calculating eeprom_wc
or if you would prefer to keep the original shift expression?

Thank you!
-Pavan
> 
> 
> > 
> >  	for (addr = 0; addr < nic->eeprom_wc; addr++) {
> >  		nic->eeprom[addr] = e100_eeprom_read(nic, &addr_len,
> > addr); @@ -804,6 +804,14 @@ static int e100_eeprom_save(struct nic
> > *nic, u16 start, u16 count)
> > 
> >  	/* Try reading with an 8-bit addr len to discover actual addr
> > len */
> >  	e100_eeprom_read(nic, &addr_len, 0);
> > +
> > +	if (!addr_len || addr_len >= 16) {
> > +		netif_err(nic, probe, nic->netdev,
> > +			"Invalid EEPROM address length %u\n",
> > +			addr_len);
> > +		return -EIO;
> > +	}
> > +
> >  	nic->eeprom_wc = 1 << addr_len;
> I'm for explicit (u16)BIT(addr_len) here too, what do you think?
> 
> > 
> >  	if (start + count >= nic->eeprom_wc)
> > --
> > 2.43.0
>