Re: [PATCH v2 1/2] Introduce flexible array struct helpers

From: Keith Packard <keithp@keithp.com>
To: Kees Cook <keescook@chromium.org>,
	"Darrick J . Wong" <djwong@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Francis Laniel <laniel_francis@privacyrequired.com>,
	Daniel Axtens <dja@axtens.net>,
	Dan Williams <dan.j.williams@intel.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Tadeusz Struk <tadeusz.struk@linaro.org>,
	Zorro Lang <zlang@redhat.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Andy Shevchenko <andriy.shevchenko@intel.com>,
	Paolo Abeni <pabeni@redhat.com>,
	linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 1/2] Introduce flexible array struct helpers
Date: Mon, 24 Oct 2022 11:35:03 -0700	[thread overview]
Message-ID: <87k04pf4tk.fsf@keithp.com> (raw)
In-Reply-To: <20221024172058.534477-1-keescook@chromium.org>

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

Kees Cook <keescook@chromium.org> writes:

> + * struct flex_array_struct_example {
> + *	...			 // arbitrary members
> + *	bounded_flex_array(
> + *		u16, part_count, // count of elements stored in "parts" below.
> + *		u32, parts	 // flexible array with elements of type u32.
> + *	);
> + * );

> + * struct flex_array_struct_example {
> + *	...		// position-sensitive members
> + *	// count of elements stored in "parts" below.
> + *	DECLARE_FAS_COUNT(u16, part_count);
> + *	..		// position-sensitive members
> + *	// flexible array with elements of type u32.
> + *	DECLARE_FAS_ARRAY(u32, parts);
> + * };

I'm sure there's a good reason, but these two macros appear to be doing
similar things and yet have very different naming conventions. Maybe:

        FAS_DECLARE_COUNT(type, name)
        FAS_DECLARE_ARRAY(type, name)
        FAS_DECLARE(size_type, size_name, array_type, array_name)

> +/* For use with flexible array structure helpers, in <linux/flex_array.h> */
> +#define __DECLARE_FAS_COUNT(TYPE, NAME)					\
> +	union {								\
> +		TYPE __flex_array_elements_count;			\
> +		TYPE NAME;						\
> +	}

How often could that second "public" member be 'const'? That would catch
places which accidentally assign to this field.

For code which does want to write to this field, is it mostly trimming
data from the end, or does it actually smash in arbitrary values? For
the former case, would it be helpful to have a test to make sure the
assigned size isn't larger than the real size (yeah, that would probably
take an extra field holding the real size), or larger than the current size?

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]