[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-kernel
Subject:    Re: [PATCH v2 1/2] Introduce flexible array struct helpers
From:       Kees Cook <keescook () chromium ! org>
Date:       2022-10-26 20:33:47
Message-ID: 202210260904.41D0BE00A () keescook
[Download RAW message or body]

On Mon, Oct 24, 2022 at 11:35:03AM -0700, Keith Packard wrote:
> 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)

Well, the custom has been for individual things, call it "DECLARE_*",
and for groups, we went with lower-case macros (e.g. struct_group()).

> 
> > +/* 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?

I don't think this'll work within arbitrary struct declarations, but it
would be nice. :)

-- 
Kees Cook
[prev in list] [next in list] [prev in thread] [next in thread]