* [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
@ 2025-05-27 8:56 ` Mikael Wessel
0 siblings, 0 replies; 10+ messages in thread
From: Mikael Wessel @ 2025-05-27 8:56 UTC (permalink / raw)
To: netdev
Cc: intel-wired-lan, torvalds, anthony.l.nguyen, przemyslaw.kitszel,
andrew, kuba, pabeni, security, stable, davem, edumazet,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
2025-05-27 8:56 ` Mikael Wessel
@ 2025-05-27 14:37 ` Loktionov, Aleksandr
-1 siblings, 0 replies; 10+ messages in thread
From: Loktionov, Aleksandr @ 2025-05-27 14:37 UTC (permalink / raw)
To: Mikael Wessel, netdev@vger.kernel.org
Cc: intel-wired-lan@lists.osuosl.org, torvalds@linuxfoundation.org,
Nguyen, Anthony L, Kitszel, Przemyslaw, 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
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
@ 2025-05-27 14:37 ` Loktionov, Aleksandr
0 siblings, 0 replies; 10+ messages in thread
From: Loktionov, Aleksandr @ 2025-05-27 14:37 UTC (permalink / raw)
To: Mikael Wessel, netdev@vger.kernel.org
Cc: intel-wired-lan@lists.osuosl.org, torvalds@linuxfoundation.org,
Nguyen, Anthony L, Kitszel, Przemyslaw, 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
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
2025-05-27 8:56 ` Mikael Wessel
(?)
(?)
@ 2025-05-27 14:43 ` Paul Menzel
2025-05-27 14:45 ` Paul Menzel
-1 siblings, 1 reply; 10+ messages in thread
From: Paul Menzel @ 2025-05-27 14:43 UTC (permalink / raw)
To: Mikael Wessel
Cc: netdev, intel-wired-lan, torvalds, anthony.l.nguyen,
przemyslaw.kitszel, andrew, kuba, pabeni, security, stable, davem,
edumazet, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
2025-05-27 14:43 ` Paul Menzel
@ 2025-05-27 14:45 ` Paul Menzel
0 siblings, 0 replies; 10+ messages in thread
From: Paul Menzel @ 2025-05-27 14:45 UTC (permalink / raw)
To: Mikael Wessel
Cc: netdev, intel-wired-lan, torvalds, anthony.l.nguyen,
przemyslaw.kitszel, andrew, kuba, pabeni, security, stable, davem,
edumazet, linux-kernel
[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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
2025-05-27 8:56 ` Mikael Wessel
@ 2025-05-27 15:52 ` Tony Nguyen
-1 siblings, 0 replies; 10+ messages in thread
From: Tony Nguyen @ 2025-05-27 15:52 UTC (permalink / raw)
To: Mikael Wessel, netdev
Cc: intel-wired-lan, torvalds, przemyslaw.kitszel, andrew, kuba,
pabeni, security, stable, davem, edumazet, linux-kernel
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++)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/1] e1000e: fix heap overflow in e1000_set_eeprom()
@ 2025-05-27 15:52 ` Tony Nguyen
0 siblings, 0 replies; 10+ messages in thread
From: Tony Nguyen @ 2025-05-27 15:52 UTC (permalink / raw)
To: Mikael Wessel, netdev
Cc: intel-wired-lan, torvalds, przemyslaw.kitszel, andrew, kuba,
pabeni, security, stable, davem, edumazet, linux-kernel
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++)
^ permalink raw reply [flat|nested] 10+ messages in thread