Message-ID: <20210728023217.GC35706@embeddedor>
Date:   Tue, 27 Jul 2021 21:32:17 -0500
From:   "Gustavo A. R. Silva" <gustavoars@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-hardening@...r.kernel.org,
        Keith Packard <keithpac@...zon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-staging@...ts.linux.dev, linux-block@...r.kernel.org,
        linux-kbuild@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: Re: [PATCH 04/64] stddef: Introduce struct_group() helper macro

On Tue, Jul 27, 2021 at 01:57:55PM -0700, Kees Cook wrote:
> Kernel code has a regular need to describe groups of members within a
> structure usually when they need to be copied or initialized separately
> from the rest of the surrounding structure. The generally accepted design
> pattern in C is to use a named sub-struct:
> 
> 	struct foo {
> 		int one;
> 		struct {
> 			int two;
> 			int three;
> 		} thing;
> 		int four;
> 	};
> 
> This would allow for traditional references and sizing:
> 
> 	memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
> 
> However, doing this would mean that referencing struct members enclosed
> by such named structs would always require including the sub-struct name
> in identifiers:
> 
> 	do_something(dst.thing.three);
> 
> This has tended to be quite inflexible, especially when such groupings
> need to be added to established code which causes huge naming churn.
> Three workarounds exist in the kernel for this problem, and each have
> other negative properties.
> 
> To avoid the naming churn, there is a design pattern of adding macro
> aliases for the named struct:
> 
> 	#define f_three thing.three
> 
> This ends up polluting the global namespace, and makes it difficult to
> search for identifiers.
> 
> Another common work-around in kernel code avoids the pollution by avoiding
> the named struct entirely, instead identifying the group's boundaries using
> either a pair of empty anonymous structs of a pair of zero-element arrays:
> 
> 	struct foo {
> 		int one;
> 		struct { } start;
> 		int two;
> 		int three;
> 		struct { } finish;
> 		int four;
> 	};
> 
> 	struct foo {
> 		int one;
> 		int start[0];
> 		int two;
> 		int three;
> 		int finish[0];
> 		int four;
> 	};
> 
> This allows code to avoid needing to use a sub-struct name for member
> references within the surrounding structure, but loses the benefits of
> being able to actually use such a struct, making it rather fragile. Using
> these requires open-coded calculation of sizes and offsets. The efforts
> made to avoid common mistakes include lots of comments, or adding various
> BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason
> about the boundaries (e.g. the "start" object looks like it's 0 bytes
> in length and is not structurally associated with "finish"), making bounds
> checking depend on open-coded calculations:
> 
> 	if (length > offsetof(struct foo, finish) -
> 		     offsetof(struct foo, start))
> 		return -EINVAL;
> 	memcpy(&dst.start, &src.start, length);
> 
> However, the vast majority of places in the kernel that operate on
> groups of members do so without any identification of the grouping,
> relying either on comments or implicit knowledge of the struct contents,
> which is even harder for the compiler to reason about, and results in
> even more fragile manual sizing, usually depending on member locations
> outside of the region (e.g. to copy "two" and "three", use the start of
> "four" to find the size):
> 
> 	BUILD_BUG_ON((offsetof(struct foo, four) <
> 		      offsetof(struct foo, two)) ||
> 		     (offsetof(struct foo, four) <
> 		      offsetof(struct foo, three));
> 	if (length > offsetof(struct foo, four) -
> 		     offsetof(struct foo, two))
> 		return -EINVAL;
> 	memcpy(&dst.two, &src.two, length);
> 
> And both of the prior two idioms additionally appear to write beyond the
> end of the referenced struct member, forcing the compiler to ignore any
> attempt to perform bounds checking.
> 
> In order to have a regular programmatic way to describe a struct
> region that can be used for references and sizing, can be examined for
> bounds checking, avoids forcing the use of intermediate identifiers,
> and avoids polluting the global namespace, introduce the struct_group()
> macro. This macro wraps the member declarations to create an anonymous
> union of an anonymous struct (no intermediate name) and a named struct
> (for references and sizing):
> 
> 	struct foo {
> 		int one;
> 		struct_group(thing,
> 			int two,
> 			int three,
> 		);
> 		int four;
> 	};
> 
> 	if (length > sizeof(src.thing))
> 		return -EINVAL;
> 	memcpy(&dst.thing, &src.thing, length);
> 	do_something(dst.three);
> 
> There are some rare cases where the resulting struct_group() needs
> attributes added, so struct_group_attr() is also introduced to allow
> for specifying struct attributes (e.g. __align(x) or __packed).
> 
> Co-developed-by: Keith Packard <keithpac@...zon.com>
> Signed-off-by: Keith Packard <keithpac@...zon.com>
> Signed-off-by: Kees Cook <keescook@...omium.org>

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

Love it! :)

Thanks
--
Gustavo

> ---
>  include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/linux/stddef.h b/include/linux/stddef.h
> index 998a4ba28eba..cf7f866944f9 100644
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef.h
> @@ -36,4 +36,38 @@ enum {
>  #define offsetofend(TYPE, MEMBER) \
>  	(offsetof(TYPE, MEMBER)	+ sizeof_field(TYPE, MEMBER))
>  
> +/**
> + * struct_group_attr(NAME, ATTRS, MEMBERS)
> + *
> + * Used to create an anonymous union of two structs with identical
> + * layout and size: one anonymous and one named. The former 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. Includes structure attributes argument.
> + *
> + * @NAME: The name of the mirrored sub-struct
> + * @ATTRS: Any struct attributes (normally empty)
> + * @MEMBERS: The member declarations for the mirrored structs
> + */
> +#define struct_group_attr(NAME, ATTRS, MEMBERS) \
> +	union { \
> +		struct { MEMBERS } ATTRS; \
> +		struct { MEMBERS } ATTRS NAME; \
> +	}
> +
> +/**
> + * struct_group(NAME, MEMBERS)
> + *
> + * Used to create an anonymous union of two structs with identical
> + * layout and size: one anonymous and one named. The former 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.
> + *
> + * @NAME: The name of the mirrored sub-struct
> + * @MEMBERS: The member declarations for the mirrored structs
> + */
> +#define struct_group(NAME, MEMBERS)	\
> +	struct_group_attr(NAME, /* no attrs */, MEMBERS)
> +
>  #endif
> -- 
> 2.30.2
> 

Powered by blists - more mailing lists