netdev - Re: [PATCH v2] Fix initializing a static union variable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 16:18:40 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Yabin Cui <yabinc@...gle.com>, Steffen Klassert <steffen.klassert@...unet.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v2] Fix initializing a static union variable

On Fri, 21 Jun 2024 at 15:36, Herbert Xu <herbert@...dor.apana.org.au> wrote:
>
> On Fri, Jun 21, 2024 at 02:18:19PM -0700, Yabin Cui wrote:
> > saddr_wildcard is a static union variable initialized with {}.
> >
> > Empty brace initialization of union types is unspecified prior to C23,
> > and even in C23, it doesn't guarantee zero initialization of all fields
> > (see sections 4.5 and 6.2 in
> > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm).
>
> What about all the other places in the kernel that use the same
> idiom? A grep shows that there are more than a hundred spots in
> the kernel where {} is used to initialise a union.

The important part is not what the standards text says - we happily
use things like inline asms that are entirely outside the standard -
but that apparently clang silently generates bogus code.

And from my admittedly _very_ limited testing, it's not that clang
always gets this wrong, but gets this wrong for a very particular
case: where the first field is smaller than the other fields.

And when the union is embedded in a struct, the struct initialization
seems to be ok from a quick test, but I might have screwed that test
up.

Now, it's still a worry, but I just wanted to point out that it's not
necessarily that *every* case is problematic.

Also, the problem Yabin found isn't actually limited to the empty
initializer. It happens even when you have an explicit zero in there.
All you need is _any_ initializer that doesn't initialize the whole
size.

End result: the "empty initializer" is a red herring and only relevant
to that standards paperwork.

So empty initializers are not relevant to the actual bug in question,
and I actually think that commit message is actively misleading in
trying to make it be about some "Linux isn't following standatrds".

But that also means that searching for empty initializers isn't going
to find all potential problem spots.

Notice how the suggested kernel patch was to remove the initializer
entirely, and just rely on "static variables are always zero" instead.

I don't know how to detect this problem case sanely, since the clang
bug occurs with non-static variables too, and with an actual non-empty
initializer too.

           Linus