netdev - Re: [PATCH net-next v4 2/2] 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: <202502190928.D2C5927D0@keescook>
Date: Wed, 19 Feb 2025 09:28:40 -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, Ilya Maximets <i.maximets@....org>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next v4 2/2] net: Add options as a flexible array to
 struct ip_tunnel_info

On Wed, Feb 19, 2025 at 04:32:56PM +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().
> 
> The layout of struct ip_tunnel_info remains the same with this patch.
> Before this patch, there was an implicit padding at the end of the
> struct, options would be written at 'info + 1' which is after the
> padding.
> This will remain the same as this patch explicitly aligns 'options'.
> The alignment is needed as the options are later casted to different
> structs, and might result in unaligned memory access.
> 
> Pahole output before this patch:
> struct ip_tunnel_info {
>     struct ip_tunnel_key       key;                  /*     0    64 */
> 
>     /* XXX last struct has 1 byte of padding */
> 
>     /* --- cacheline 1 boundary (64 bytes) --- */
>     struct ip_tunnel_encap     encap;                /*    64     8 */
>     struct dst_cache           dst_cache;            /*    72    16 */
>     u8                         options_len;          /*    88     1 */
>     u8                         mode;                 /*    89     1 */
> 
>     /* size: 96, cachelines: 2, members: 5 */
>     /* padding: 6 */
>     /* paddings: 1, sum paddings: 1 */
>     /* last cacheline: 32 bytes */
> };
> 
> Pahole output after this patch:
> struct ip_tunnel_info {
>     struct ip_tunnel_key       key;                  /*     0    64 */
> 
>     /* XXX last struct has 1 byte of padding */
> 
>     /* --- cacheline 1 boundary (64 bytes) --- */
>     struct ip_tunnel_encap     encap;                /*    64     8 */
>     struct dst_cache           dst_cache;            /*    72    16 */
>     u8                         options_len;          /*    88     1 */
>     u8                         mode;                 /*    89     1 */
> 
>     /* XXX 6 bytes hole, try to pack */
> 
>     u8                         options[] __attribute__((__aligned__(16))); /*    96     0 */
> 
>     /* size: 96, cachelines: 2, members: 6 */
>     /* sum members: 90, holes: 1, sum holes: 6 */
>     /* paddings: 1, sum paddings: 1 */
>     /* forced alignments: 1, forced holes: 1, sum forced holes: 6 */
>     /* last cacheline: 32 bytes */
> } __attribute__((__aligned__(16)));
> 
> [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>

Thanks for these updates and the pahole output. :)

Reviewed-by: Kees Cook <kees@...nel.org>

-- 
Kees Cook