On 8/7/2025 9:20 AM, Paul Menzel wrote:
Dear Vitaly, dear Mikael,
Thank you for the patch.
Am 07.08.25 um 06:05 schrieb 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.
Do you have a reproducer for this issue?
This patch was originally created by Mikael Wessel. Since the requested
changes weren’t addressed for several months, I decided to take over and
submit an updated version. Therefore, I’m not aware of a specific
reproducer or report for the issue.
You can refer to the original submission here:
https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver
(currently for ICH9 devices only)")
Co-developed-by: Mikael Wessel <[email protected]>
Signed-off-by: Mikael Wessel <[email protected]>
Signed-off-by: Vitaly Lifshits <[email protected]>
---
v2: Use check_add_overflow for boundary checking
---
drivers/net/ethernet/intel/e1000e/ethtool.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/
net/ethernet/intel/e1000e/ethtool.c
index 9364bc2b4eb1..2bfbc7fd9559 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -550,6 +550,7 @@ static int e1000_set_eeprom(struct net_device
*netdev,
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
u16 *eeprom_buff;
+ u32 total_len;
Use `unsigned int`? Although the type of `max_len` is signed for
whatever reason.
Yes, I agree—it seems incorrect. I’ll change both total_len and max_len
to size_t in v3.
void *ptr;
int max_len;
int first_word;
@@ -569,6 +570,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;
+
A few places in the Linux kernel also add `unlikely` in front of it.
Would that make sense here?
I don’t think adding unlikely is appropriate in this case. This function
is called infrequently, so the optimization would be negligible. Also,
since the input comes from userspace, any value could be passed in,
making it hard to classify as “unlikely.”
Also, how is the error case logged?
I chose not to log the error to maintain consistency with the rest of
the function, which doesn’t log similar errors.
first_word = eeprom->offset >> 1;
last_word = (eeprom->offset + eeprom->len - 1) >> 1;
eeprom_buff = kmalloc(max_len, GFP_KERNEL);
Kind regards,
Paul
Thank you for the review.
