netdev - 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 for Android: free password hash cracker in your pocket

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

Message-ID: <929107ae-2ed3-4d3b-b719-62b7ccd60413@intel.com>
Date: Fri, 21 Jun 2024 13:01:35 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Yabin Cui <yabinc@...gle.com>, Nick Desaulniers <ndesaulniers@...gle.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<llvm@...ts.linux.dev>, 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>
Subject: Re: [PATCH] Fix initializing a static union variable

On 6/20/24 20:17, Yabin Cui 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,
> clang only initializes the first field as zero, but the bits
> corresponding to other (larger) members are undefined.

Nice finding!

You have to add a Fixes tag, perhaps:
Fixes: 08ec9af1c062 ("xfrm: Fix xfrm_state_find() wrt. wildcard source 
address.")

And the other thing is that I would change the order in the union, to
have largest element as the first. Would be best to also add such check
into static analysis tool/s.

> 
> 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;