linux-kernel - Re: [PATCH v2] stddef: make __struct

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: <57428517-33ee-4f5a-a8a2-7a016ce5eba8@embeddedor.com>
Date: Thu, 19 Dec 2024 09:41:52 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>,
 Kees Cook <kees@...nel.org>
Cc: Christopher Ferris <cferris@...gle.com>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Erick Archer <erick.archer@...look.com>, Keith Packard <keithp@...thp.com>,
 Dan Williams <dan.j.williams@...el.com>, linux-hardening@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] stddef: make __struct_group() UAPI C++-friendly



On 19/12/24 07:57, Alexander Lobakin wrote:
> For the most part of the C++ history, it couldn't have type
> declarations inside anonymous unions for different reasons. At the
> same time, __struct_group() relies on the latters, so when the @TAG
> argument is not empty, C++ code doesn't want to build (even under
> `extern "C"`):
> 
> ../linux/include/uapi/linux/pkt_cls.h:25:24: error:
> 'struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,' invalid;
> an anonymous union may only have public non-static data members
> [-fpermissive]
> 
> The safest way to fix this without trying to switch standards (which
> is impossible in UAPI anyway) etc., is to disable tag declaration
> for that language. This won't break anything since for now it's not
> buildable at all.
> Use a separate definition for __struct_group() when __cplusplus is
> defined to mitigate the error, including the version from tools/.
> 
> Fixes: 50d7bd38c3aa ("stddef: Introduce struct_group() helper macro")
> Reported-by: Christopher Ferris <cferris@...gle.com>
> Closes: https://lore.kernel.org/linux-hardening/Z1HZpe3WE5As8UAz@google.com
> Suggested-by: Kees Cook <kees@...nel.org> # __struct_group_tag()
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@...nel.org>

Thanks!
-Gustavo

> ---
>  From v1[0]:
> * wrap @TAG into __struct_group_tag() (Kees).
> 
> [0] https://lore.kernel.org/linux-hardening/20241218145758.701008-1-aleksander.lobakin@intel.com
> ---
>   include/uapi/linux/stddef.h       | 13 ++++++++++---
>   tools/include/uapi/linux/stddef.h | 15 +++++++++++----
>   2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
> index 58154117d9b0..a6fce46aeb37 100644
> --- a/include/uapi/linux/stddef.h
> +++ b/include/uapi/linux/stddef.h
> @@ -8,6 +8,13 @@
>   #define __always_inline inline
>   #endif
>   
> +/* Not all C++ standards support type declarations inside an anonymous union */
> +#ifndef __cplusplus
> +#define __struct_group_tag(TAG)		TAG
> +#else
> +#define __struct_group_tag(TAG)
> +#endif
> +
>   /**
>    * __struct_group() - Create a mirrored named and anonyomous struct
>    *
> @@ -20,13 +27,13 @@
>    * and size: one anonymous and one named. The former's members can be used
>    * normally without sub-struct naming, and the latter can be used to
>    * reason about the start, end, and size of the group of struct members.
> - * The named struct can also be explicitly tagged for layer reuse, as well
> - * as both having struct attributes appended.
> + * The named struct can also be explicitly tagged for layer reuse (C only),
> + * as well as both having struct attributes appended.
>    */
>   #define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>   	union { \
>   		struct { MEMBERS } ATTRS; \
> -		struct TAG { MEMBERS } ATTRS NAME; \
> +		struct __struct_group_tag(TAG) { MEMBERS } ATTRS NAME; \
>   	} ATTRS
>   
>   #ifdef __cplusplus
> diff --git a/tools/include/uapi/linux/stddef.h b/tools/include/uapi/linux/stddef.h
> index bb6ea517efb5..c53cde425406 100644
> --- a/tools/include/uapi/linux/stddef.h
> +++ b/tools/include/uapi/linux/stddef.h
> @@ -8,6 +8,13 @@
>   #define __always_inline __inline__
>   #endif
>   
> +/* Not all C++ standards support type declarations inside an anonymous union */
> +#ifndef __cplusplus
> +#define __struct_group_tag(TAG)		TAG
> +#else
> +#define __struct_group_tag(TAG)
> +#endif
> +
>   /**
>    * __struct_group() - Create a mirrored named and anonyomous struct
>    *
> @@ -20,14 +27,14 @@
>    * and size: one anonymous and one named. The former's members can be used
>    * normally without sub-struct naming, and the latter can be used to
>    * reason about the start, end, and size of the group of struct members.
> - * The named struct can also be explicitly tagged for layer reuse, as well
> - * as both having struct attributes appended.
> + * The named struct can also be explicitly tagged for layer reuse (C only),
> + * as well as both having struct attributes appended.
>    */
>   #define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>   	union { \
>   		struct { MEMBERS } ATTRS; \
> -		struct TAG { MEMBERS } ATTRS NAME; \
> -	}
> +		struct __struct_group_tag(TAG) { MEMBERS } ATTRS NAME; \
> +	} ATTRS
>   
>   /**
>    * __DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union