Re: [PATCH net-next v2 2/2] e1000e: limit endianness conversion to boundary words

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 3/25/2026 8:16 AM, Agalakov Daniil wrote:
> [Why]
> In e1000_set_eeprom(), the eeprom_buff is allocated to hold a range of
> words. However, only the boundary words (the first and the last) are
> populated from the EEPROM if the write request is not word-aligned.
> The words in the middle of the buffer remain uninitialized because they
> are intended to be completely overwritten by the new data via memcpy().
> 
> The previous implementation had a loop that performed le16_to_cpus()
> on the entire buffer. This resulted in endianness conversion being
> performed on uninitialized memory for all interior words.
> 
> Fix this by converting the endianness only for the boundary words
> immediately after they are successfully read from the EEPROM.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Co-developed-by: Iskhakov Daniil <dish@amicon.ru>
> Signed-off-by: Iskhakov Daniil <dish@amicon.ru>
> Signed-off-by: Agalakov Daniil <ade@amicon.ru>
> ---
> v2:
>   - Split from the original bugfix series and targeted at 'net-text'.
>   - Removed the Fixes: tag; limiting the conversion scope is an
>     improvement to avoid unnecessary processing of uninitialized memory.
>   - Improved commit description for clarity.
>   - Note on e1000e: this driver already contains the necessary return
>     value checks for EEPROM reads, so only the endianness conversion
>     cleanup is included for e1000e.
> 
>   drivers/net/ethernet/intel/e1000e/ethtool.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index dbed30943ef4..785d89477c43 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -583,13 +583,21 @@ static int e1000_set_eeprom(struct net_device *netdev,
>   		/* need read/modify/write of first changed EEPROM word */
>   		/* only the second byte of the word is being modified */
>   		ret_val = e1000_read_nvm(hw, first_word, 1, &eeprom_buff[0]);
> +
> +		/* Device's eeprom is always little-endian, word addressable */
> +		le16_to_cpus(&eeprom_buff[0]);

I think the v1 was better. We should check the ret_val first so we don't 
do this conversion on error.

> +
>   		ptr++;
>   	}
> -	if (((eeprom->offset + eeprom->len) & 1) && (!ret_val))
> +	if (((eeprom->offset + eeprom->len) & 1) && (!ret_val)) {
>   		/* need read/modify/write of last changed EEPROM word */
>   		/* only the first byte of the word is being modified */
>   		ret_val = e1000_read_nvm(hw, last_word, 1,
>   					 &eeprom_buff[last_word - first_word]);

Same here.

Also, same for the e1000 version.

> +	
> +		/* Device's eeprom is always little-endian, word addressable */
> +		le16_to_cpus(&eeprom_buff[last_word - first_word]);
> +	}
>   
>   	if (ret_val)
>   		goto out;

Also, AI review reports:

The patch adds le16_to_cpus() conversion immediately after reading the
boundary words, but doesn't this cause double endianness conversion?

Looking at the original code before this patch, there should be loops
after the "goto out" section that convert the entire buffer:

     /* Device's eeprom is always little-endian, word addressable */
     for (i = 0; i < last_word - first_word + 1; i++)
         le16_to_cpus(&eeprom_buff[i]);

And then another loop before writing back:

     for (i = 0; i < last_word - first_word + 1; i++)
         cpu_to_le16s(&eeprom_buff[i]);

If those loops are still present, the boundary words at indices 0 and
(last_word - first_word) would be converted twice - once here at lines
588 and 597, then again in the loop. On big-endian systems, converting
LE->CPU->LE would byte-swap the boundary words compared to the user's
intended values. Does the patch also remove those loops, or is this
causing data corruption on big-endian architectures?

Thanks,
Tony