linux-hardening - Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z1HZpe3WE5As8UAz@google.com>
Date: Thu, 5 Dec 2024 08:49:41 -0800
From: Nick Desaulniers <ndesaulniers@...gle.com>
To: Kees Cook <kees@...nel.org>
Cc: "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
	Jakub Kicinski <kuba@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Michael Chan <michael.chan@...adcom.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Potnuri Bharat Teja <bharat@...lsio.com>,
	Christian Benvenuti <benve@...co.com>,
	Satish Kharat <satishkh@...co.com>,
	Manish Chopra <manishc@...vell.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
	cferris@...gle.com, android-llvm-dev@...gle.com
Subject: Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in
 struct ethtool_link_settings

On Fri, Nov 15, 2024 at 12:38:55PM -0800, Kees Cook wrote:
> On Tue, Nov 12, 2024 at 07:08:32PM -0600, Gustavo A. R. Silva wrote:
> > 
> > 
> > On 11/11/24 16:22, Gustavo A. R. Silva wrote:
> > > 
> > > 
> > > On 09/11/24 12:02, Jakub Kicinski wrote:
> > > > On Tue, 29 Oct 2024 15:55:35 -0600 Gustavo A. R. Silva wrote:
> > > > > Use the `__struct_group()` helper to create a new tagged
> > > > > `struct ethtool_link_settings_hdr`. This structure groups together
> > > > > all the members of the flexible `struct ethtool_link_settings`
> > > > > except the flexible array. As a result, the array is effectively
> > > > > separated from the rest of the members without modifying the memory
> > > > > layout of the flexible structure.
> > > > > 
> > > > > This new tagged struct will be used to fix problematic declarations
> > > > > of middle-flex-arrays in composite structs[1].
> > > > 
> > > > Possibly a very noob question, but I'm updating a C++ library with
> > > > new headers and I think this makes it no longer compile.
> > > > 
> > > > $ cat > /tmp/t.cpp<<EOF
> > > > extern "C" {
> > > > #include "include/uapi/linux/ethtool.h"
> > > > }
> > > > int func() { return 0; }
> > > > EOF
> > > > 
> > > > $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2
> > > > In file included from /usr/include/linux/posix_types.h:5,
> > > >                   from /usr/include/linux/types.h:9,
> > > >                   from ../linux/include/uapi/linux/ethtool.h:18,
> > > >                   from /tmp/t.cpp:2:
> > > > ../linux/include/uapi/linux/ethtool.h:2515:24: error: ‘struct
> > > > ethtool_link_settings::<unnamed union>::ethtool_link_settings_hdr’
> > > > invalid; an anonymous union may only have public non-static data
> > > > members [-fpermissive]
> > > >   2515 |         __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */,
> > > >        |                        ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > > 
> > > > 
> > 
> > This seems to work with Clang:
> > 
> > $ clang++-18 -fms-extensions /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2
> > 
> > However, `-fms-extensions` doesn't seem to work for this case with GCC:
> > 
> > https://godbolt.org/z/1shsPhz3s
> 
> Hm, we can't break UAPI even for C++, so even if we had compiler options
> that would make it work, it's really unfriendly to userspace to make all
> the projects there suddenly start needing to use it.
> 
> I think this means we just can't use tagged struct groups in UAPI. :(
> 
> I have what I think is a much simpler solution. Sending it now...

What was the follow up?

cferris@ is reporting something similar for linux/uapi/linux/pkt_cls.h in the
structure tc_u32_sel definition, found while updating the kernel headers in
Android. Maybe that file/definition needs the same follow up?

~Nick