netdev - Re: [RFC net-next 1/2] overflow: add DECLARE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202308011403.E0A8D25CE@keescook>
Date: Tue, 1 Aug 2023 15:31:10 -0700
From: Kees Cook <keescook@...omium.org>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Jacob Keller <jacob.e.keller@...el.com>,
	intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	Alexander Lobakin <aleksander.lobakin@...el.com>
Subject: Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack
 allocs

On Tue, Aug 01, 2023 at 01:19:22PM +0200, Przemek Kitszel wrote:
> Add DECLARE_FLEX() macro for on-stack allocations of structs with
> flexible array member.

I like this idea!

One terminology nit: I think this should be called "DEFINE_...", since
it's a specific instantiation. Other macros in the kernel seem to confuse
this a lot, though. Yay naming.

> Using underlying array for on-stack storage lets us to declare known
> on compile-time structures without kzalloc().

Hmpf, this appears to immediately trip over any (future) use of
__counted_by()[1] for these (since the counted-by member would be
initialized to zero), but I think I have a solution. (See below.)

> 
> Actual usage for ice driver is in next patch of the series.
> 
> Note that "struct" kw and "*" char is moved to the caller, to both:
> have shorter macro name, and have more natural type specification
> in the driver code (IOW not hiding an actual type of var).
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> ---
>  include/linux/overflow.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index f9b60313eaea..403b7ec120a2 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -309,4 +309,18 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  #define struct_size_t(type, member, count)					\
>  	struct_size((type *)NULL, member, count)
>  
> +/**
> + * DECLARE_FLEX() - Declare an on-stack instance of structure with trailing
> + * flexible array.
> + * @type: Pointer to structure type, including "struct" keyword and "*" char.
> + * @name: Name for a (pointer) variable to create.
> + * @member: Name of the array member.
> + * @count: Number of elements in the array; must be compile-time const.
> + *
> + * Declare an instance of structure *@...e with trailing flexible array.
> + */
> +#define DECLARE_FLEX(type, name, member, count)					\
> +	u8 name##_buf[struct_size((type)NULL, member, count)] __aligned(8) = {};\
> +	type name = (type)&name##_buf
> +
>  #endif /* __LINUX_OVERFLOW_H */

I was disappointed to discover that only global (static) initializers
would work for a flex array member. :(

i.e. this works:

struct foo {
    unsigned long flags;
    unsigned char count;
    int array[] __counted_by(count);
};

struct foo global = {
    .count = 1,
    .array = { 0 },
};

But I can't do that on the stack. :P So, yes, it seems like the u8 array
trick is needed.

It looks like Alexander already suggested this, and I agree, instead of
__aligned(8), please use "__aligned(_Alignof(type))".

As for "*" or not, I would tend to agree that always requiring "*" when
using the macro seems redundant.

Initially I was struggling to make __counted_by work, but it seems we can
use an initializer for that member, as long as we don't touch the flexible
array member in the initializer. So we just need to add the counted-by
member to the macro, and use a union to do the initialization. And if
we take the address of the union (and not the struct within it), the
compiler will see the correct object size with __builtin_object_size:

#define DEFINE_FLEX(type, name, flex, counter, count) \
    union { \
        u8   bytes[struct_size_t(type, flex, count)]; \
        type obj; \
    } name##_u __aligned(_Alignof(type)) = { .obj.counter = count }; \
    /* take address of whole union to get the correct __builtin_object_size */ \
    type *name = (type *)&name##_u

i.e. __builtin_object_size(name, 1) (as used by FORTIFY_SOURCE, etc)
works correctly here, but breaks (sees a zero-sized flex array member)
if this macro ends with:

    type *name = &name##_u.obj

-Kees

[1] https://git.kernel.org/linus/dd06e72e68bcb4070ef211be100d2896e236c8fb

-- 
Kees Cook