[PATCH v2 net 0/1] e1000e: fix heap overflow in e1000_set_eeprom()

[PATCH v2 net 0/1] e1000e: fix heap overflow in e1000_set_eeprom()

[Mikael Wessel]

v2: patch the correct write helper and add bounds-checking; v1
    mistakenly guarded e1000_get_eeprom() (read path).

---

Mikael Wessel (1):
  e1000e: fix heap overflow in e1000_set_eeprom()

 drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.48.1

[PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()

[Mikael Wessel]

The ETHTOOL_SETEEPROM ioctl copies user data into a kmalloc'ed buffer
without validating eeprom->len and eeprom->offset.  A CAP_NET_ADMIN
user can overflow the heap and crash the kernel or gain code execution.

Validate length and offset before memcpy().

Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)")
Reported-by: Mikael Wessel <post@mikaelkw.online>
Signed-off-by: Mikael Wessel <post@mikaelkw.online>
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 9364bc2b4eb1..98e541e39730 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -596,6 +596,9 @@ static int e1000_set_eeprom(struct net_device *netdev,
 	for (i = 0; i < last_word - first_word + 1; i++)
 		le16_to_cpus(&eeprom_buff[i]);
 
+        if (eeprom->len > max_len ||
+            eeprom->offset > max_len - eeprom->len)
+                return -EINVAL;
 	memcpy(ptr, bytes, eeprom->len);
 
 	for (i = 0; i < last_word - first_word + 1; i++)
-- 
2.48.1

RE: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Mikael Wessel
> Sent: Tuesday, May 27, 2025 10:56 AM
> To: netdev@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; torvalds@linuxfoundation.org;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; andrew@lunn.ch; kuba@kernel.org;
> pabeni@redhat.com; security@kernel.org; stable@vger.kernel.org;
> davem@davemloft.net; edumazet@google.com; linux-
> kernel@vger.kernel.org; Mikael Wessel <post@mikaelkw.online>
> Subject: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in
> e1000_set_eeprom()
> 
> The ETHTOOL_SETEEPROM ioctl copies user data into a kmalloc'ed buffer
> without validating eeprom->len and eeprom->offset.  A CAP_NET_ADMIN
> user can overflow the heap and crash the kernel or gain code
> execution.
> 
> Validate length and offset before memcpy().
> 
> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver
> (currently for ICH9 devices only)")
> Reported-by: Mikael Wessel <post@mikaelkw.online>
> Signed-off-by: Mikael Wessel <post@mikaelkw.online>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 9364bc2b4eb1..98e541e39730 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -596,6 +596,9 @@ static int e1000_set_eeprom(struct net_device
> *netdev,
>  	for (i = 0; i < last_word - first_word + 1; i++)
>  		le16_to_cpus(&eeprom_buff[i]);
> 
> +        if (eeprom->len > max_len ||
> +            eeprom->offset > max_len - eeprom->len)
> +                return -EINVAL;
>  	memcpy(ptr, bytes, eeprom->len);
> 
>  	for (i = 0; i < last_word - first_word + 1; i++)
> --
> 2.48.1

Re: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()

[Paul Menzel]

Dear Mikael,


Thank you for your patch.

Am 27.05.25 um 10:56 schrieb Mikael Wessel:
> The ETHTOOL_SETEEPROM ioctl copies user data into a kmalloc'ed buffer
> without validating eeprom->len and eeprom->offset.  A CAP_NET_ADMIN
> user can overflow the heap and crash the kernel or gain code execution.
> 
> Validate length and offset before memcpy().
> 
> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)")
> Reported-by: Mikael Wessel <post@mikaelkw.online>
> Signed-off-by: Mikael Wessel <post@mikaelkw.online>
> Cc: stable@vger.kernel.org
> ---
>   drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 9364bc2b4eb1..98e541e39730 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -596,6 +596,9 @@ static int e1000_set_eeprom(struct net_device *netdev,
>   	for (i = 0; i < last_word - first_word + 1; i++)
>   		le16_to_cpus(&eeprom_buff[i]);
>   
> +        if (eeprom->len > max_len ||
> +            eeprom->offset > max_len - eeprom->len)
> +                return -EINVAL;

I think you used spaces instead of tabs for indentation. It’d be great 
if you could fix this, and send v3 tomorrow. Running 
`scripts/checkpatch.pl` with the patch as an argument, should catch 
these things.

>   	memcpy(ptr, bytes, eeprom->len);
>   
>   	for (i = 0; i < last_word - first_word + 1; i++)


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()

[Paul Menzel]

[one addition]

Am 27.05.25 um 16:43 schrieb Paul Menzel:
> Dear Mikael,
> 
> 
> Thank you for your patch.
> 
> Am 27.05.25 um 10:56 schrieb Mikael Wessel:
>> The ETHTOOL_SETEEPROM ioctl copies user data into a kmalloc'ed buffer
>> without validating eeprom->len and eeprom->offset.  A CAP_NET_ADMIN
>> user can overflow the heap and crash the kernel or gain code execution.
>>
>> Validate length and offset before memcpy().
>>
>> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver 
>> (currently for ICH9 devices only)")
>> Reported-by: Mikael Wessel <post@mikaelkw.online>
>> Signed-off-by: Mikael Wessel <post@mikaelkw.online>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/ 
>> net/ethernet/intel/e1000e/ethtool.c
>> index 9364bc2b4eb1..98e541e39730 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
>> @@ -596,6 +596,9 @@ static int e1000_set_eeprom(struct net_device 
>> *netdev,
>>       for (i = 0; i < last_word - first_word + 1; i++)
>>           le16_to_cpus(&eeprom_buff[i]);
>> +        if (eeprom->len > max_len ||
>> +            eeprom->offset > max_len - eeprom->len)
>> +                return -EINVAL;
> 
> I think you used spaces instead of tabs for indentation. It’d be great 
> if you could fix this, and send v3 tomorrow. Running `scripts/ 
> checkpatch.pl` with the patch as an argument, should catch these things.

Should a warning/error be logged if the condition is true?

>>       memcpy(ptr, bytes, eeprom->len);
>>       for (i = 0; i < last_word - first_word + 1; i++)
> 
> 
> Kind regards,
> 
> Paul

Re: [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()

[Tony Nguyen]

On 5/27/2025 1:56 AM, Mikael Wessel wrote:
> The ETHTOOL_SETEEPROM ioctl copies user data into a kmalloc'ed buffer
> without validating eeprom->len and eeprom->offset.  A CAP_NET_ADMIN
> user can overflow the heap and crash the kernel or gain code execution.
> 
> Validate length and offset before memcpy().
> 
> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)")
> Reported-by: Mikael Wessel <post@mikaelkw.online>
> Signed-off-by: Mikael Wessel <post@mikaelkw.online>
> Cc: stable@vger.kernel.org
> ---
>   drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 9364bc2b4eb1..98e541e39730 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -596,6 +596,9 @@ static int e1000_set_eeprom(struct net_device *netdev,
>   	for (i = 0; i < last_word - first_word + 1; i++)
>   		le16_to_cpus(&eeprom_buff[i]);
>   
> +        if (eeprom->len > max_len ||
> +            eeprom->offset > max_len - eeprom->len)
> +                return -EINVAL;

This is going to cause 'eeprom_buff' to leak. You should use the goto 
out, however, seems like these checks can be moved up before the 
allocation is done. Also, indentation looks off.

Thanks,
Tony

>   	memcpy(ptr, bytes, eeprom->len);
>   
>   	for (i = 0; i < last_word - first_word + 1; i++)