[Intel-wired-lan] [PATCH iwl-net v3 1/1] e1000e: fix heap overflow in e1000_set_eeprom

[Intel-wired-lan] [PATCH iwl-net v3 1/1] e1000e: fix heap overflow in e1000_set_eeprom

[Vitaly Lifshits]

Fix a possible heap overflow in e1000_set_eeprom function by adding
input validation for the requested length of the change in the EEPROM.
In addition, change the variable type from int to size_t for better
code practices and rearrange declarations to RCT.

Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)")
Co-developed-by: Mikael Wessel <post@mikaelkw.online>
Signed-off-by: Mikael Wessel <post@mikaelkw.online>
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
---
v3: Change max_len and total_len to size_t
v2: Use check_add_overflow for boundary checking
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 9364bc2b4eb1..d7f25f007e8e 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -549,12 +549,12 @@ 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;
 	u16 *eeprom_buff;
-	void *ptr;
-	int max_len;
+	int ret_val = 0;
 	int first_word;
 	int last_word;
-	int ret_val = 0;
+	void *ptr;
 	u16 i;
 
 	if (eeprom->len == 0)
@@ -569,6 +569,10 @@ static int e1000_set_eeprom(struct net_device *netdev,
 
 	max_len = hw->nvm.word_size * 2;
 
+	if (check_add_overflow(eeprom->offset, eeprom->len, &total_len) ||
+	    total_len > max_len)
+		return -EINVAL;
+
 	first_word = eeprom->offset >> 1;
 	last_word = (eeprom->offset + eeprom->len - 1) >> 1;
 	eeprom_buff = kmalloc(max_len, GFP_KERNEL);
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net v3 1/1] e1000e: fix heap overflow in e1000_set_eeprom

[Przemek Kitszel]

On 8/14/25 09:18, Vitaly Lifshits wrote:
> Fix a possible heap overflow in e1000_set_eeprom function by adding
> input validation for the requested length of the change in the EEPROM.
> In addition, change the variable type from int to size_t for better

thats good

> code practices and rearrange declarations to RCT.

I would avoid, especially that this is now majority of the changes in
the first chunk. But OTOH, does not matter that much for rarely updated
driver.

> 
> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)")
> Co-developed-by: Mikael Wessel <post@mikaelkw.online>
> Signed-off-by: Mikael Wessel <post@mikaelkw.online>
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> ---
> v3: Change max_len and total_len to size_t
> v2: Use check_add_overflow for boundary checking
> ---
>   drivers/net/ethernet/intel/e1000e/ethtool.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 9364bc2b4eb1..d7f25f007e8e 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -549,12 +549,12 @@ 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;
>   	u16 *eeprom_buff;
> -	void *ptr;
> -	int max_len;
> +	int ret_val = 0;
>   	int first_word;
>   	int last_word;
> -	int ret_val = 0;
> +	void *ptr;
>   	u16 i;
>   
>   	if (eeprom->len == 0)
> @@ -569,6 +569,10 @@ static int e1000_set_eeprom(struct net_device *netdev,
>   
>   	max_len = hw->nvm.word_size * 2;
>   
> +	if (check_add_overflow(eeprom->offset, eeprom->len, &total_len) ||
> +	    total_len > max_len)
> +		return -EINVAL;

I would differentiate error codes (EINVAL is already in use for this
function), and in general prefer to use broader range.

What about EFBIG or E2BIG?

> +
>   	first_word = eeprom->offset >> 1;
>   	last_word = (eeprom->offset + eeprom->len - 1) >> 1;
>   	eeprom_buff = kmalloc(max_len, GFP_KERNEL);