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

[Kees Cook <kees@kernel.org>]

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

Thanks for these updates and the pahole output. :)

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

-- 
Kees Cook