e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

[PATCH] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

Posted by Ivy Lopez 1 month, 3 weeks ago

e1000_read_nvm_spt() reads two adjacent 16-bit NVM words as one
32-bit dword for efficiency. When deciding whether to use the
shadow RAM value for the high word (data[i + 1]), it incorrectly
checks the "modified" flag of the low word (shadow_ram[offset + i])
instead of the high word's own flag (shadow_ram[offset + i + 1]).

This can cause silent NVM shadow RAM corruption:
 - if only the high word is marked modified, its pending write is
   lost and the stale flash value is returned instead
 - if only the low word is marked modified, the high word is
   incorrectly overwritten with the low word's shadow value

This function is used on all PCH chips from SPT onward (SPT, CNP,
TGP, ADP, MTP, etc). The bug has been present since the dword-read
optimization was introduced for these chips.

Note: this was found via code review while investigating a separate
reproducible packet loss issue on I219-LM (SPT+ PCH). Testing showed
this fix does not resolve that particular symptom, but the flag
check is still a genuine logic bug that can corrupt shadow RAM
contents and should be fixed regardless.

Fixes: 79849ebc0e06 ("e1000e: initial support for i219")
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index aa90e0ce8aca..87cd1cdc6f10 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -3473,7 +3473,7 @@ static s32 e1000_read_nvm_spt(struct e1000_hw *hw, u16 offset, u16 words,
 				    dev_spec->shadow_ram[offset + i].value;
 			else
 				data[i] = (u16)(dword & 0xFFFF);
-			if (dev_spec->shadow_ram[offset + i].modified)
+			if (dev_spec->shadow_ram[offset + i + 1].modified)
 				data[i + 1] =
 				    dev_spec->shadow_ram[offset + i + 1].value;
 			else
-- 
2.55.0

RE: [Intel-wired-lan] [PATCH] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

Posted by Loktionov, Aleksandr 1 month, 3 weeks ago


> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Ivy Lopez
> Sent: Thursday, August 6, 2026 7:25 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ivy Lopez
> <skunkolee@gmail.com>
> Subject: [Intel-wired-lan] [PATCH] e1000e: fix incorrect modified flag
> check in e1000_read_nvm_spt()
> 
> e1000_read_nvm_spt() reads two adjacent 16-bit NVM words as one 32-bit
> dword for efficiency. When deciding whether to use the shadow RAM
> value for the high word (data[i + 1]), it incorrectly checks the
> "modified" flag of the low word (shadow_ram[offset + i]) instead of
> the high word's own flag (shadow_ram[offset + i + 1]).
> 
> This can cause silent NVM shadow RAM corruption:
>  - if only the high word is marked modified, its pending write is
>    lost and the stale flash value is returned instead
>  - if only the low word is marked modified, the high word is
>    incorrectly overwritten with the low word's shadow value
> 
> This function is used on all PCH chips from SPT onward (SPT, CNP, TGP,
> ADP, MTP, etc). The bug has been present since the dword-read
> optimization was introduced for these chips.
> 
> Note: this was found via code review while investigating a separate
> reproducible packet loss issue on I219-LM (SPT+ PCH). Testing showed
> this fix does not resolve that particular symptom, but the flag check
> is still a genuine logic bug that can corrupt shadow RAM contents and
> should be fixed regardless.
> 
> Fixes: 79849ebc0e06 ("e1000e: initial support for i219")
> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index aa90e0ce8aca..87cd1cdc6f10 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -3473,7 +3473,7 @@ static s32 e1000_read_nvm_spt(struct e1000_hw
> *hw, u16 offset, u16 words,
>  				    dev_spec->shadow_ram[offset + i].value;
>  			else
>  				data[i] = (u16)(dword & 0xFFFF);
> -			if (dev_spec->shadow_ram[offset + i].modified)
> +			if (dev_spec->shadow_ram[offset + i +
> 1].modified)
>  				data[i + 1] =
>  				    dev_spec->shadow_ram[offset + i +
> 1].value;
>  			else
> --
> 2.55.0

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [Intel-wired-lan] [PATCH] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

Posted by Ruinskiy, Dima 1 month, 3 weeks ago

On 07/08/2026 13:56, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>> Of Ivy Lopez
>> Sent: Thursday, August 6, 2026 7:25 PM
>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
>> Przemyslaw <przemyslaw.kitszel@intel.com>
>> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
>> kuba@kernel.org; pabeni@redhat.com; intel-wired-lan@lists.osuosl.org;
>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ivy Lopez
>> <skunkolee@gmail.com>
>> Subject: [Intel-wired-lan] [PATCH] e1000e: fix incorrect modified flag
>> check in e1000_read_nvm_spt()
>>
>> e1000_read_nvm_spt() reads two adjacent 16-bit NVM words as one 32-bit
>> dword for efficiency. When deciding whether to use the shadow RAM
>> value for the high word (data[i + 1]), it incorrectly checks the
>> "modified" flag of the low word (shadow_ram[offset + i]) instead of
>> the high word's own flag (shadow_ram[offset + i + 1]).
>>
>> This can cause silent NVM shadow RAM corruption:
>>   - if only the high word is marked modified, its pending write is
>>     lost and the stale flash value is returned instead
>>   - if only the low word is marked modified, the high word is
>>     incorrectly overwritten with the low word's shadow value
>>
>> This function is used on all PCH chips from SPT onward (SPT, CNP, TGP,
>> ADP, MTP, etc). The bug has been present since the dword-read
>> optimization was introduced for these chips.
>>
>> Note: this was found via code review while investigating a separate
>> reproducible packet loss issue on I219-LM (SPT+ PCH). Testing showed
>> this fix does not resolve that particular symptom, but the flag check
>> is still a genuine logic bug that can corrupt shadow RAM contents and
>> should be fixed regardless.
>>
>> Fixes: 79849ebc0e06 ("e1000e: initial support for i219")
>> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> index aa90e0ce8aca..87cd1cdc6f10 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> @@ -3473,7 +3473,7 @@ static s32 e1000_read_nvm_spt(struct e1000_hw
>> *hw, u16 offset, u16 words,
>>   				    dev_spec->shadow_ram[offset + i].value;
>>   			else
>>   				data[i] = (u16)(dword & 0xFFFF);
>> -			if (dev_spec->shadow_ram[offset + i].modified)
>> +			if (dev_spec->shadow_ram[offset + i +
>> 1].modified)
>>   				data[i + 1] =
>>   				    dev_spec->shadow_ram[offset + i +
>> 1].value;
>>   			else
>> --
>> 2.55.0
> 
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
Reviewed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>

Re: [Intel-wired-lan] [PATCH] e1000e: fix incorrect modified flag check in e1000_read_nvm_spt()

Posted by Kadosh, MoriyaX 1 month, 2 weeks ago


On 09/08/2026 12:21, Ruinskiy, Dima wrote:
> On 07/08/2026 13:56, Loktionov, Aleksandr wrote:
>>
>>
>>> -----Original Message-----
>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>>> Of Ivy Lopez
>>> Sent: Thursday, August 6, 2026 7:25 PM
>>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
>>> Przemyslaw <przemyslaw.kitszel@intel.com>
>>> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
>>> kuba@kernel.org; pabeni@redhat.com; intel-wired-lan@lists.osuosl.org;
>>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ivy Lopez
>>> <skunkolee@gmail.com>
>>> Subject: [Intel-wired-lan] [PATCH] e1000e: fix incorrect modified flag
>>> check in e1000_read_nvm_spt()
>>>
>>> e1000_read_nvm_spt() reads two adjacent 16-bit NVM words as one 32-bit
>>> dword for efficiency. When deciding whether to use the shadow RAM
>>> value for the high word (data[i + 1]), it incorrectly checks the
>>> "modified" flag of the low word (shadow_ram[offset + i]) instead of
>>> the high word's own flag (shadow_ram[offset + i + 1]).
>>>
>>> This can cause silent NVM shadow RAM corruption:
>>>   - if only the high word is marked modified, its pending write is
>>>     lost and the stale flash value is returned instead
>>>   - if only the low word is marked modified, the high word is
>>>     incorrectly overwritten with the low word's shadow value
>>>
>>> This function is used on all PCH chips from SPT onward (SPT, CNP, TGP,
>>> ADP, MTP, etc). The bug has been present since the dword-read
>>> optimization was introduced for these chips.
>>>
>>> Note: this was found via code review while investigating a separate
>>> reproducible packet loss issue on I219-LM (SPT+ PCH). Testing showed
>>> this fix does not resolve that particular symptom, but the flag check
>>> is still a genuine logic bug that can corrupt shadow RAM contents and
>>> should be fixed regardless.
>>>
>>> Fixes: 79849ebc0e06 ("e1000e: initial support for i219")
>>> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
>>> ---
>>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> index aa90e0ce8aca..87cd1cdc6f10 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> @@ -3473,7 +3473,7 @@ static s32 e1000_read_nvm_spt(struct e1000_hw
>>> *hw, u16 offset, u16 words,
>>>                       dev_spec->shadow_ram[offset + i].value;
>>>               else
>>>                   data[i] = (u16)(dword & 0xFFFF);
>>> -            if (dev_spec->shadow_ram[offset + i].modified)
>>> +            if (dev_spec->shadow_ram[offset + i +
>>> 1].modified)
>>>                   data[i + 1] =
>>>                       dev_spec->shadow_ram[offset + i +
>>> 1].value;
>>>               else
>>> -- 
>>> 2.55.0
>>
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>
> Reviewed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> 
Tested-by: Moriya Kadosh <moriyax.kadosh@intel.com>