RE: [PATCH 5.10.y] e1000e: fix EEPROM length types for overflow checks

RE: [PATCH 5.10.y] e1000e: fix EEPROM length types for overflow checks

[Farber, Eliav]

> On Wed, Sep 10, 2025 at 05:31:38PM +0000, Eliav Farber wrote:
>> Fix a compilation failure when warnings are treated as errors:
>>
>> drivers/net/ethernet/intel/e1000e/ethtool.c: In function ‘e1000_set_eeprom’:
>> ./include/linux/overflow.h:71:15: error: comparison of distinct pointer types lacks a cast [-Werror]
>>    71 |  (void) (&__a == __d);   \
>>       |               ^~
>> drivers/net/ethernet/intel/e1000e/ethtool.c:582:6: note: in expansion of macro ‘check_add_overflow’
>>   582 |  if (check_add_overflow(eeprom->offset, eeprom->len, &total_len) ||
>>       |      ^~~~~~~~~~~~~~~~~~
>>
>> To fix this, change total_len and max_len from size_t to u32 in 
>> e1000_set_eeprom().
>> The check_add_overflow() helper requires that the first two operands 
>> and the pointer to the result (third operand) all have the same type.
>> On 64-bit builds, using size_t caused a mismatch with the u32 fields
>> eeprom->offset and eeprom->len, leading to type check failures.
>>
>> Fixes: ce8829d3d44b ("e1000e: fix heap overflow in e1000_set_eeprom")
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>>  drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c 
>> b/drivers/net/ethernet/intel/e1000e/ethtool.c
>> index 4aca854783e2..584378291f3f 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
>> @@ -559,7 +559,7 @@ static int e1000_set_eeprom(struct net_device 
>> *netdev,  {
>>       struct e1000_adapter *adapter = netdev_priv(netdev);
>>       struct e1000_hw *hw = &adapter->hw;
>> -     size_t total_len, max_len;
>> +     u32 total_len, max_len;
>>       u16 *eeprom_buff;
>>       int ret_val = 0;
>>       int first_word;
>> --
>> 2.47.3
>>
>
> Why is this not needed in Linus's tree?
Kernel 5.10.243 enforces the same type, but this enforcement is
absent from 5.15.192 and later:
/*
 * For simplicity and code hygiene, the fallback code below insists on
 * a, b and *d having the same type (similar to the min() and max()
 * macros), whereas gcc's type-generic overflow checkers accept
 * different types. Hence we don't just make check_add_overflow an
 * alias for __builtin_add_overflow, but add type checks similar to
 * below.
 */
#define check_add_overflow(a, b, d) __must_check_overflow(({	\
 
> Also, why is it not cc: stable@vger.kernel.org?
Added to cc.

---
Regards, Eliav

RE: [PATCH 5.10.y] e1000e: fix EEPROM length types for overflow checks

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Farber, Eliav
> Sent: Thursday, September 11, 2025 8:14 AM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: jesse.brandeburg@intel.com; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem@davemloft.net; kuba@kernel.org;
> Lifshits, Vitaly <vitaly.lifshits@intel.com>; post@mikaelkw.online;
> intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Chocron, Jonathan <jonnyc@amazon.com>;
> stable@vger.kernel.org; Farber, Eliav <farbere@amazon.com>
> Subject: Re: [Intel-wired-lan] [PATCH 5.10.y] e1000e: fix EEPROM
> length types for overflow checks
> 

...

> >> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> b/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> index 4aca854783e2..584378291f3f 100644
> >> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> @@ -559,7 +559,7 @@ static int e1000_set_eeprom(struct net_device
> >> *netdev,  {
> >>       struct e1000_adapter *adapter = netdev_priv(netdev);
> >>       struct e1000_hw *hw = &adapter->hw;
> >> -     size_t total_len, max_len;
> >> +     u32 total_len, max_len;
I'd like to recommend adding a comment to prevent future regressions, like:
/* Use u32 to match types in check_add_overflow() with eeprom->offset and eeprom->len */
Anyway, it's good commit.
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>


...
> > Also, why is it not cc: stable@vger.kernel.org?
> Added to cc.
> 
> ---
> Regards, Eliav

Re: [PATCH 5.10.y] e1000e: fix EEPROM length types for overflow checks

[Greg KH]

On Thu, Sep 11, 2025 at 06:13:33AM +0000, Farber, Eliav wrote:
> > On Wed, Sep 10, 2025 at 05:31:38PM +0000, Eliav Farber wrote:
> >> Fix a compilation failure when warnings are treated as errors:
> >>
> >> drivers/net/ethernet/intel/e1000e/ethtool.c: In function ‘e1000_set_eeprom’:
> >> ./include/linux/overflow.h:71:15: error: comparison of distinct pointer types lacks a cast [-Werror]
> >>    71 |  (void) (&__a == __d);   \
> >>       |               ^~
> >> drivers/net/ethernet/intel/e1000e/ethtool.c:582:6: note: in expansion of macro ‘check_add_overflow’
> >>   582 |  if (check_add_overflow(eeprom->offset, eeprom->len, &total_len) ||
> >>       |      ^~~~~~~~~~~~~~~~~~
> >>
> >> To fix this, change total_len and max_len from size_t to u32 in 
> >> e1000_set_eeprom().
> >> The check_add_overflow() helper requires that the first two operands 
> >> and the pointer to the result (third operand) all have the same type.
> >> On 64-bit builds, using size_t caused a mismatch with the u32 fields
> >> eeprom->offset and eeprom->len, leading to type check failures.
> >>
> >> Fixes: ce8829d3d44b ("e1000e: fix heap overflow in e1000_set_eeprom")
> >> Signed-off-by: Eliav Farber <farbere@amazon.com>
> >> ---
> >>  drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c 
> >> b/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> index 4aca854783e2..584378291f3f 100644
> >> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> >> @@ -559,7 +559,7 @@ static int e1000_set_eeprom(struct net_device 
> >> *netdev,  {
> >>       struct e1000_adapter *adapter = netdev_priv(netdev);
> >>       struct e1000_hw *hw = &adapter->hw;
> >> -     size_t total_len, max_len;
> >> +     u32 total_len, max_len;
> >>       u16 *eeprom_buff;
> >>       int ret_val = 0;
> >>       int first_word;
> >> --
> >> 2.47.3
> >>
> >
> > Why is this not needed in Linus's tree?
> Kernel 5.10.243 enforces the same type, but this enforcement is
> absent from 5.15.192 and later:
> /*
>  * For simplicity and code hygiene, the fallback code below insists on
>  * a, b and *d having the same type (similar to the min() and max()
>  * macros), whereas gcc's type-generic overflow checkers accept
>  * different types. Hence we don't just make check_add_overflow an
>  * alias for __builtin_add_overflow, but add type checks similar to
>  * below.
>  */
> #define check_add_overflow(a, b, d) __must_check_overflow(({	\

Yeah, the min() build warning mess is slowly propagating back to older
kernels over time as we take these types of fixes backwards.  I count 3
such new warnings in the new 5.10 release, not just this single one.

Overall, how about fixing this up so it doesn't happen anymore by
backporting the min() logic instead?  That should solve this build
warning, and keep it from happening again in the future?  I did that for
newer kernel branches, but never got around to it for these.

thanks,

greg k-h

RE: [PATCH 5.10.y] e1000e: fix EEPROM length types for overflow checks

[Farber, Eliav]

> On Thu, Sep 11, 2025 at 06:13:33AM +0000, Farber, Eliav wrote:
> > > On Wed, Sep 10, 2025 at 05:31:38PM +0000, Eliav Farber wrote:
> > >> Fix a compilation failure when warnings are treated as errors:
> > >>
> > >> drivers/net/ethernet/intel/e1000e/ethtool.c: In function ‘e1000_set_eeprom’:
> > >> ./include/linux/overflow.h:71:15: error: comparison of distinct pointer types lacks a cast [-Werror]
> > >>    71 |  (void) (&__a == __d);   \
> > >>       |               ^~
> > >> drivers/net/ethernet/intel/e1000e/ethtool.c:582:6: note: in expansion of macro ‘check_add_overflow’
> > >>   582 |  if (check_add_overflow(eeprom->offset, eeprom->len, &total_len) ||
> > >>       |      ^~~~~~~~~~~~~~~~~~
> > >>
> > >> To fix this, change total_len and max_len from size_t to u32 in
> > >> e1000_set_eeprom().
> > >> The check_add_overflow() helper requires that the first two operands
> > >> and the pointer to the result (third operand) all have the same type.
> > >> On 64-bit builds, using size_t caused a mismatch with the u32 fields
> > >> eeprom->offset and eeprom->len, leading to type check failures.
> > >>
> > >> Fixes: ce8829d3d44b ("e1000e: fix heap overflow in e1000_set_eeprom")
> > >> Signed-off-by: Eliav Farber <farbere@amazon.com>
> > >> ---
> > >>  drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > >> b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > >> index 4aca854783e2..584378291f3f 100644
> > >> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > >> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > >> @@ -559,7 +559,7 @@ static int e1000_set_eeprom(struct net_device
> > >> *netdev,  {
> > >>       struct e1000_adapter *adapter = netdev_priv(netdev);
> > >>       struct e1000_hw *hw = &adapter->hw;
> > >> -     size_t total_len, max_len;
> > >> +     u32 total_len, max_len;
> > >>       u16 *eeprom_buff;
> > >>       int ret_val = 0;
> > >>       int first_word;
> > >> --
> > >> 2.47.3
> > >>
> > >
> > > Why is this not needed in Linus's tree?
> > Kernel 5.10.243 enforces the same type, but this enforcement is
> > absent from 5.15.192 and later:
> > /*
> >  * For simplicity and code hygiene, the fallback code below insists on
> >  * a, b and *d having the same type (similar to the min() and max()
> >  * macros), whereas gcc's type-generic overflow checkers accept
> >  * different types. Hence we don't just make check_add_overflow an
> >  * alias for __builtin_add_overflow, but add type checks similar to
> >  * below.
> >  */
> > #define check_add_overflow(a, b, d) __must_check_overflow(({  \
>
> Yeah, the min() build warning mess is slowly propagating back to older
> kernels over time as we take these types of fixes backwards.  I count 3
> such new warnings in the new 5.10 release, not just this single one.
>
> Overall, how about fixing this up so it doesn't happen anymore by
> backporting the min() logic instead?  That should solve this build
> warning, and keep it from happening again in the future?  I did that for
> newer kernel branches, but never got around to it for these.

I did backporting of 4 commits to bring include/linux/overflow.h in
line with v5.15.193 in order to pull commit 1d1ac8244c22 ("overflow:
Allow mixed type arguments").
I'll also check what can be done for include/linux/minmax.h.

---
Regards, Eliav

Re: [PATCH 5.10.y] e1000e: fix EEPROM length types for overflow checks

[Greg KH]

On Fri, Sep 12, 2025 at 01:07:35PM +0000, Farber, Eliav wrote:
> > On Thu, Sep 11, 2025 at 06:13:33AM +0000, Farber, Eliav wrote:
> > > > On Wed, Sep 10, 2025 at 05:31:38PM +0000, Eliav Farber wrote:
> > > >> Fix a compilation failure when warnings are treated as errors:
> > > >>
> > > >> drivers/net/ethernet/intel/e1000e/ethtool.c: In function ‘e1000_set_eeprom’:
> > > >> ./include/linux/overflow.h:71:15: error: comparison of distinct pointer types lacks a cast [-Werror]
> > > >>    71 |  (void) (&__a == __d);   \
> > > >>       |               ^~
> > > >> drivers/net/ethernet/intel/e1000e/ethtool.c:582:6: note: in expansion of macro ‘check_add_overflow’
> > > >>   582 |  if (check_add_overflow(eeprom->offset, eeprom->len, &total_len) ||
> > > >>       |      ^~~~~~~~~~~~~~~~~~
> > > >>
> > > >> To fix this, change total_len and max_len from size_t to u32 in
> > > >> e1000_set_eeprom().
> > > >> The check_add_overflow() helper requires that the first two operands
> > > >> and the pointer to the result (third operand) all have the same type.
> > > >> On 64-bit builds, using size_t caused a mismatch with the u32 fields
> > > >> eeprom->offset and eeprom->len, leading to type check failures.
> > > >>
> > > >> Fixes: ce8829d3d44b ("e1000e: fix heap overflow in e1000_set_eeprom")
> > > >> Signed-off-by: Eliav Farber <farbere@amazon.com>
> > > >> ---
> > > >>  drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> > > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > >> b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > >> index 4aca854783e2..584378291f3f 100644
> > > >> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > >> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > >> @@ -559,7 +559,7 @@ static int e1000_set_eeprom(struct net_device
> > > >> *netdev,  {
> > > >>       struct e1000_adapter *adapter = netdev_priv(netdev);
> > > >>       struct e1000_hw *hw = &adapter->hw;
> > > >> -     size_t total_len, max_len;
> > > >> +     u32 total_len, max_len;
> > > >>       u16 *eeprom_buff;
> > > >>       int ret_val = 0;
> > > >>       int first_word;
> > > >> --
> > > >> 2.47.3
> > > >>
> > > >
> > > > Why is this not needed in Linus's tree?
> > > Kernel 5.10.243 enforces the same type, but this enforcement is
> > > absent from 5.15.192 and later:
> > > /*
> > >  * For simplicity and code hygiene, the fallback code below insists on
> > >  * a, b and *d having the same type (similar to the min() and max()
> > >  * macros), whereas gcc's type-generic overflow checkers accept
> > >  * different types. Hence we don't just make check_add_overflow an
> > >  * alias for __builtin_add_overflow, but add type checks similar to
> > >  * below.
> > >  */
> > > #define check_add_overflow(a, b, d) __must_check_overflow(({  \
> >
> > Yeah, the min() build warning mess is slowly propagating back to older
> > kernels over time as we take these types of fixes backwards.  I count 3
> > such new warnings in the new 5.10 release, not just this single one.
> >
> > Overall, how about fixing this up so it doesn't happen anymore by
> > backporting the min() logic instead?  That should solve this build
> > warning, and keep it from happening again in the future?  I did that for
> > newer kernel branches, but never got around to it for these.
> 
> I did backporting of 4 commits to bring include/linux/overflow.h in
> line with v5.15.193 in order to pull commit 1d1ac8244c22 ("overflow:
> Allow mixed type arguments").
> I'll also check what can be done for include/linux/minmax.h.

Very cool, thank you!