linux-kernel - Re: [PATCH] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOd=DkqejWW=CTmaSi8gqopvCsVtj+63ppuR0nw==M26imA@mail.gmail.com>
Date: Thu, 20 Jun 2024 12:47:42 -0700
From: Nick Desaulniers <ndesaulniers@...gle.com>
To: Yabin Cui <yabinc@...gle.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>, Herbert Xu <herbert@...dor.apana.org.au>, 
	"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>, Bill Wendling <morbo@...gle.com>, 
	Justin Stitt <justinstitt@...gle.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, 
	"Ballman, Aaron" <aaron.ballman@...el.com>
Subject: Re: [PATCH] Fix initializing a static union variable

On Thu, Jun 20, 2024 at 12:31 PM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> On Thu, Jun 20, 2024 at 11:17 AM Yabin Cui <yabinc@...gle.com> wrote:
> >
> > saddr_wildcard is a static union variable initialized with {}.
> > But c11 standard doesn't guarantee initializing all fields as
> > zero for this case. As in https://godbolt.org/z/rWvdv6aEx,
>
> Specifically, it sounds like C99+ is just the first member of the
> union, which is dumb since that may not necessarily be the largest
> variant.  Can you find the specific relevant wording from a pre-c23
> spec?
>
> > clang only initializes the first field as zero, but the bits
> > corresponding to other (larger) members are undefined.
>
> Oh, that sucks!
>
> Reading through the internal report on this is fascinating!  Nice job
> tracking down the issue!  It sounds like if we can aggressively inline
> the users of this partially initialized value, then the UB from
> control flow on the partially initialized value can result in
> Android's kernel network tests failing.  It might be good to include
> more info on "why this is a problem" in the commit message.
>
> https://godbolt.org/z/hxnT1PTWo more clearly demonstrates the issue, IMO.
>
> TIL that C23 clarifies this, but clang still doesn't have the correct
> codegen then for -std=c23.  Can you please find or file a bug about
> this, then add a link to it in the commit message?
>
> It might be interesting to link to the specific section of n3096 that
> clarifies this, or if there was a corresponding defect report sent to
> ISO about this.  Maybe something from
> https://www.open-std.org/jtc1/sc22/wg14/www/wg14_document_log.htm
> discusses this?

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3011.htm

https://clang.llvm.org/c_status.html mentions that n3011 was addressed
by clang-17, but based on my godbolt link above, it seems perhaps not?

6.7.10.2 of n3096 (c23) defines "empty initialization" (which wasn't
defined in older standards).

Ah, reading

n2310 (c17) 6.7.9.10:

```
If an object that has static or thread storage duration is not
initialized explicitly, then:
...
— if it is a union, the first named member is initialized
(recursively) according to these rules, and
any padding is initialized to zero bits;
```

Specifically, "the first named member" was a terrible mistake in the language.

Yikes! Might want to quote that in the commit message.

>
> Can you also please (find or) file a bug against clang about this? A
> compiler diagnostic would be very very helpful here, since `= {};` is
> such a common idiom.
>
> Patch LGTM, but I think more context can be provided in the commit
> message in a v2 that helps reviewers follow along with what's going on
> here.
>
> >
> > Signed-off-by: Yabin Cui <yabinc@...gle.com>
> > ---
> >  net/xfrm/xfrm_state.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 649bb739df0d..9bc69d703e5c 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -1139,7 +1139,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >                 struct xfrm_policy *pol, int *err,
> >                 unsigned short family, u32 if_id)
> >  {
> > -       static xfrm_address_t saddr_wildcard = { };
> > +       static const xfrm_address_t saddr_wildcard;
> >         struct net *net = xp_net(pol);
> >         unsigned int h, h_wildcard;
> >         struct xfrm_state *x, *x0, *to_put;
> > --
> > 2.45.2.741.gdbec12cfda-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers