Re: [PATCH net 2/3] e1000: fix endianness conversion of uninitialized words

[Fedor Pchelkin]

On Wed, 25. Mar 16:01, Jacob Keller wrote:
> On 3/25/2026 8:19 AM, Fedor Pchelkin wrote:
> > Hi,
> > 
> > On Tue, 24. Mar 16:26, Tony Nguyen wrote:
> >> On 3/18/2026 5:05 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.
> >>>
> >>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >>
> >> While this is definitely better, I'm not sure there's a bug here since it's
> >> being immediately overwritten. Seems like this patch would be better going
> >> to *-next as an improvement.
> > 
> > It's worth stating in the commit message that the uninitialized memory is
> > touched with le16_to_cpus() in the loop only on BE systems.  Little-endian
> > ones are not affected - le16_to_cpus() is a no-op there.
> > 
> > Anyway, for the BE case, touching and manipulating uninit memory bytes is
> > still in general considered a bug, even if this memory is overwritten a
> > few lines after that.  I guess if KMSAN supported big-endian architectures,
> > it would hit this, and that wouldn't be a false-positive.
> > 
> > I'm not aware of the details on how you treat the bugs in these drivers
> > for BE-systems: maybe they aren't prioritized and then would occasionnaly
> > go as -next material.  But, again, this situation looks like a real bug
> > worth fixing on BE-systems.
> > 
> 
> Typically the bar for a fixes and a net change is that it requires some
> user visible behavioral impact. That's where the hesitance on our end
> comes from: how does this cause a user-visible bug?
> 
> If there are truly user-visible impact on BE system for touching and
> manipulating the uninitialized memory, then it makes sense to go to net
> for me. I guess "KASAN/UBSAN complains you touched uninitialized memory"
> would be such a bug.

My first thought was that reading and writing uninitialized memory is just
undefined behavior in C.  It's hard to say what some compiler might invent
here.

The current situation is not trivial though because there is a chunk of
uninit memory allocated with kmalloc() to which we have a valid pointer,
and we are swapping the order of uninitialized bytes inside this chunk,
i.e. reading/writing uninitialized memory.  Basically it should just go
down to swapping garbage values without any suspicious compiler
optimizations - I don't think the compiler is aware that kmalloc() returns
uninit memory.


KMSAN actually supports one big-endian arch - that's s390 (the only arch
except x86 that KMSAN supports).  I've tried quickly to trigger the splat
out of curiosity, but booting the KMSAN-enabled kernel with s390 QEMU TCG
looks like taking forever so unfortunately no specific results here.

KASAN/UBSAN don't catch error patterns of this type.

> 
> Alternatively: is there a risk of some side channel method to capture
> that uninitialized data and leak kernel memory? I don't recall enough to
> know whether that would be an issue here...
> 
> I don't personally have an objection to going through net (besides
> fixing the commit hash for the Fixes: tag on the patch that was wrong)
> since its an obvious fix.

Thanks for the feedback.  I see Daniil has already sent some newer
versions targeting -next.  The problem in question is rather obscure about
severity of its consequences but spending some time on investigating this
now I'm more inclined to think there should be no opportunity for UB here
and it's okay to process the patch like Tony suggested..