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

[Kees Cook]

On Thu, May 05, 2022 at 03:16:19PM +0200, Johannes Berg wrote:
> On Wed, 2022-05-04 at 08:38 -0700, Kees Cook wrote:
> > 
> > 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.
> 
> A lot of them are, and many could be, though not all.

Yeah, I did a pass through them for the coming v2. Only a few have the
struct order as part of an apparent hardware interface.

> > 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.
> 
> I guess I don't see that happening :)

Well ... it's on my roadmap. ;) I want it for -fsanitize=array-bounds so
that dynamic array indexing can be checked too. (Right now we can do
constant-sized array index bounds checking at runtime, but the much
harder to find problems tend to come from flex arrays.)

> > 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.
> 
> Yeah, dunno, I guess I'm slightly more on the side of not requiring it,
> since we don't do the same for kmalloc() etc. and probably really
> wouldn't want to add kmalloc_s() that does it ;-)

Well, I dislike all the *alloc APIs. :P

> I mean, you _could_ go there:
> 
> int kmalloc_s(void **ptr, size_t size, gfp_t gfp)

Oh, and I really do (though as a macro, not a "real" function), since
having type introspection would be _extremely_ useful. Though maybe it
needs to be through some kind of type-of-lvalue thing...

https://github.com/KSPP/linux/issues/189
https://github.com/KSPP/linux/issues/87

> So I'm not really sure why this aspect here should need to be different,
> except of course that you already need the input argument for the magic.

Right, and trying to move the kernel code closer to a form where the
compiler can take more of the burden of handling code safety.

> And btw, while I was writing it down I was looking to see if it should
> be "size_t elements" or "size_t len" (like memcpy), it took me some time
> to figure out, and I was looking at the examples:
> 
>  1) most of them actually use __u8 or some variant thereof, so you
>     could probably add an even simpler macro like
>        BOUNDED_FLEX_DATA(int, bytes, data)
>     which has the u8 type internally.

I didn't want these helpers to be "opinionated" about their types (just
their API), so while it's true u8 is usually "good enough", I don't
think it's common enough to make a special case for.

>  2) Unless I'm confusing myself, you got the firewire change wrong,
>     because __mem_to_flex_dup takes the "elements_count", but the
>     memcpy() there wasn't multiplied by the sizeof(element)? Or maybe
>     the fact that it was declared as __u32 header[0] is wrong, and it
>     should be __u8, but it's all very confusing, and I'm really not
>     sure about this at all.

Yes indeed; thanks for catching that. In fact, it's not a strict flex
array struct, since, as you say, it's measuring bytes, not elements.
Yeah, I'll see if that needs to be adjusted/dropped, etc.

> One "perhaps you'll laugh me out of the room" suggestion might be to
> actually be able to initialize the whole thing too?
> 
> mydata = flex_struct_alloc(mydata, GFP_KERNEL,
>                            variable_data, variable_len,
>                            .member = 1,
>                            .another = 2);
> 
> (the ordering can't really be otherwise since you have to use
> __VA_ARGS__).

Oooh, that's a cool idea for the API. Hmmmm.

> That might reduce some more code too, though I guess it's quite some
> additional magic ... :)

Yay preprocessor magic!

> I was going to point to struct cfg80211_bss_ies, but I realize now
> they're RCU-managed, so we never resize them anyway ... So maybe it's
> less common than I thought it might be.
> 
> I suppose you know better since you converted a lot of stuff already :-)

Well, I've seen a lot of fragile code (usually in the form of
exploitable flaws around flex arrays) and they do mostly look the same.
Not everything fits perfectly into the forms this API tries to address,
but my goal is to get it fitting well enough, and the weird stuff can be
more carefully examined -- they're easier to find and audit if all the
others are nicely wrapped up in some fancy flex*() API.

Thanks for your thoughts on all of this! I'll continue to work on a v2...

-Kees

-- 
Kees Cook