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

[Kees Cook <kees@kernel.org>]

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@kernel.org>
> Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> Signed-off-by: Gal Pressman <gal@nvidia.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