netdev - Re: [PATCH net-next] net: Add options as a flexible array to struct ip_tunnel

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: <202502110942.8DE626C@keescook>
Date: Tue, 11 Feb 2025 09:49:27 -0800
From: Kees Cook <kees@...nel.org>
To: Gal Pressman <gal@...dia.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Tariq Toukan <tariqt@...dia.com>,
	Louis Peens <louis.peens@...igine.com>,
	Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
	Pravin B Shelar <pshelar@....org>, Yotam Gigi <yotam.gi@...il.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>, dev@...nvswitch.org,
	linux-hardening@...r.kernel.org, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next] net: Add options as a flexible array to struct
 ip_tunnel_info

On Sun, Feb 09, 2025 at 12:18:53PM +0200, Gal Pressman wrote:
> Remove the hidden assumption that options are allocated at the end of
> the struct, and teach the compiler about them using a flexible array.
> 
> With this, we can revert the unsafe_memcpy() call we have in
> tun_dst_unclone() [1], and resolve the false field-spanning write
> warning caused by the memcpy() in ip_tunnel_info_opts_set().
> 
> Note that this patch changes the layout of struct ip_tunnel_info since
> there is padding at the end of the struct.
> Before this, options would be written at 'info + 1' which is after the
> padding.
> After this patch, options are written right after 'mode' field (into the
> padding).
> 
> [1] Commit 13cfd6a6d7ac ("net: Silence false field-spanning write warning in metadata_dst memcpy")
> 
> Link: https://lore.kernel.org/all/53D1D353-B8F6-4ADC-8F29-8C48A7C9C6F1@kernel.org/
> Suggested-by: Kees Cook <kees@...nel.org>
> Reviewed-by: Cosmin Ratiu <cratiu@...dia.com>
> Reviewed-by: Tariq Toukan <tariqt@...dia.com>
> Signed-off-by: Gal Pressman <gal@...dia.com>
> [...]
> @@ -107,6 +101,7 @@ struct ip_tunnel_info {
>  #endif
>  	u8			options_len;
>  	u8			mode;
> +	u8			options[] __counted_by(options_len);
>  };

I see __counted_by being added here (thank you).

> [...]
> @@ -659,7 +654,7 @@ static inline void ip_tunnel_info_opts_set(struct ip_tunnel_info *info,
>  {
>  	info->options_len = len;
>  	if (len > 0) {
> -		memcpy(ip_tunnel_info_opts(info), from, len);
> +		memcpy(info->options, from, len);
>  		ip_tunnel_flags_or(info->key.tun_flags, info->key.tun_flags,
>  				   flags);
>  	}

And I see info->options_len being set here before the copy into
info_>options. What validates that "info" was allocated with enough
space to hold "len" here? I would have expected this as allocation time
instead of here.

> diff --git a/net/core/dst.c b/net/core/dst.c
> index 9552a90d4772..d981c295a48e 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -286,7 +286,8 @@ struct metadata_dst *metadata_dst_alloc(u8 optslen, enum metadata_type type,
>  {
>  	struct metadata_dst *md_dst;
>  
> -	md_dst = kmalloc(sizeof(*md_dst) + optslen, flags);
> +	md_dst = kmalloc(struct_size(md_dst, u.tun_info.options, optslen),
> +			 flags);
>  	if (!md_dst)
>  		return NULL;
>  

I don't see options_len being set here? I assume since it's sub-type
specific. I'd expect the type to be validated (i.e. optslen must == 0
unless this is a struct ip_tunnel_info, and if so, set options_len here
instead of in ip_tunnel_info_opts_set().

Everything else looks very good, though, yes, I would agree with the
alignment comments made down-thread. This was "accidentally correct"
before in the sense that the end of the struct would be padded for
alignment, but isn't guaranteed to be true with an explicit u8 array.
The output of "pahole -C struct ip_tunnel_info" before/after should
answer any questions, though.

-Kees

-- 
Kees Cook