Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers

[Kees Cook <keescook@xxxxxxxxxxxx>]

On Wed, May 04, 2022 at 09:25:56AM +0200, Johannes Berg wrote:
> On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote:
> > 
> > For example, using the most complicated helper, mem_to_flex_dup():
> > 
> >     /* Flexible array struct with members identified. */
> >     struct something {
> >         int mode;
> >         DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, how_many);
> >         unsigned long flags;
> >         DECLARE_FLEX_ARRAY_ELEMENTS(u32, value);
> 
> In many cases, the order of the elements doesn't really matter, so maybe
> it'd be nicer to be able to write it as something like
> 
> DECLARE_FLEX_STRUCT(something,
>       int mode;
>       unsigned long flags;
>       ,
>       int, how_many,
>       u32, value);
> 
> perhaps? OK, that doesn't seem so nice either.
> 
> Maybe
> 
> struct something {
>       int mode;
>       unsigned long flags;
>       FLEX_ARRAY(
>               int, how_many,
>               u32, value
>       );
> };

Yeah, I mention some of my exploration of this idea in the sibling reply:
https://lore.kernel.org/linux-hardening/202205040730.161645EC@keescook/#t

It seemed like requiring a structure be rearranged to take advantage of
the "automatic layout introspection" wasn't very friendly. On the other
hand, looking at the examples, most of them are already neighboring
members. Hmmm.

> or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and
> DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases
> where the struct layout is not the most important thing (or it's already
> at the end anyway).

The names aren't great, but I wanted to distinguish "elements" as the
array not the count. Yay naming.

However, perhaps the solution is to have _both_. i.e using
BOUNDED_FLEX_ARRAY(count_type, count_name, array_type, array_name) for
the "neighboring" case, and the DECLARE...{ELEMENTS,COUNT} for the
"split" case.

And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include
the count_name too, so both methods could be "forward portable" to a
future where C grew the syntax for bounded flex arrays.

> 
> >     struct something *instance = NULL;
> >     int rc;
> > 
> >     rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL);
> >     if (rc)
> >         return rc;
> 
> This seems rather awkward, having to set it to NULL, then checking rc
> (and possibly needing a separate variable for it), etc.

I think the errno return is completely required. I had an earlier version
of this that was much more like a drop-in replacement for memcpy that
would just truncate or panic, and when I had it all together, I could
just imagine hearing Linus telling me to start over because it was unsafe
(truncation may be just as bad as overflow) and disruptive ("never BUG"),
and that it should be recoverable. So, I rewrote it all to return a
__must_check errno.

Requiring instance to be NULL is debatable, but I feel pretty strongly
about it because it does handle a class of mistakes (resource leaks),
and it's not much of a burden to require a known-good starting state.

> But I can understand how you arrived at this:
>  - need to pass instance or &instance or such for typeof()
>    or offsetof() or such

Right.

>  - instance = mem_to_flex_dup(instance, ...)
>    looks too much like it would actually dup 'instance', rather than
>    'byte_array'

And I need an errno output to keep imaginary Linus happy. :)

>  - if you pass &instance anyway, checking for NULL is simple and adds a
>    bit of safety

Right.

> but still, honestly, I don't like it. As APIs go, it feels a bit
> cumbersome and awkward to use, and you really need everyone to use this,
> and not say "uh what, I'll memcpy() instead".

Sure, and I have tried to get it down as small as possible. The earlier
"just put all the member names in every call" version was horrid. :P I
realize it's more work to check errno, but the memcpy() API we've all
been trained to use is just plain dangerous. I don't think it's
unreasonable to ask people to retrain themselves to avoid it. All that
said, yes, I want it to be as friendly as possible.

> Maybe there should also be a realloc() version of it?

Sure! Seems reasonable. I'd like to see the code pattern for this
though. Do you have any examples? Most of what I'd been able to find for
the fragile memcpy() usage was just basic serialize/deserialize or
direct copying.

> > +/** __fas_bytes - Calculate potential size of flexible array structure
> 
> I think you forgot "\n *" in many cases here after "/**".

Oops! Yes, thank you. I'll fix these.

-Kees

-- 
Kees Cook