netdev - Re: [ovs-dev] [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: <fe814549-3bd4-4ef6-8e7d-9d21626766e1@nvidia.com>
Date: Sun, 9 Feb 2025 21:37:59 +0200
From: Gal Pressman <gal@...dia.com>
To: Ilya Maximets <i.maximets@....org>, 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>
Cc: dev@...nvswitch.org, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Jiri Pirko <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>,
 Kees Cook <kees@...nel.org>, David Ahern <dsahern@...nel.org>,
 Yotam Gigi <yotam.gi@...il.com>, Tariq Toukan <tariqt@...dia.com>,
 linux-hardening@...r.kernel.org, Simon Horman <horms@...nel.org>,
 Cong Wang <xiyou.wangcong@...il.com>, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [ovs-dev] [PATCH net-next] net: Add options as a flexible array
 to struct ip_tunnel_info

Hi Ilya, thanks for the review.

On 09/02/2025 18:21, Ilya Maximets wrote:
> On 2/9/25 11:18, Gal Pressman via dev 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).
> 
> This doesn't sound like a safe thing to do.  'info + 1' ensures that the
> options are aligned the same way as the struct ip_tunnel_info itself.

What is special about the alignment of struct ip_tunnel_info? What are
you assuming it to be, and how is it related to whatever alignment the
options need?

> In many places in the code, the options are cast into a specific tunnel
> options type that may require sufficient alignment.  And the alignment can
> no longer be guaranteed once the options are put directly after the 'mode'.

What guaranteed it was aligned before? A hidden assumption that a u64 is
hidden somewhere in ip_tunnel_info?

> May cause crashes on some architectures as well as performance impact on
> others.
> 
> Should the alignment attribute be also added to the field?

Align to what?
To the first field of every potential options type? To eight bytes?