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: <ec63946a-c28e-8b3d-0efe-47b2638b846b@intel.com>
Date: Fri, 4 Aug 2023 17:49:00 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>, Kees Cook
	<keescook@...omium.org>
CC: Jacob Keller <jacob.e.keller@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>
Subject: Re: [RFC net-next 1/2] overflow: add DECLARE_FLEX() for on-stack
 allocs

From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Date: Fri, 4 Aug 2023 12:59:08 +0200

> On 8/2/23 00:31, Kees Cook wrote:
>> 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.
> 
> Thanks, makes sense!
> 
>>
>>> 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;
> 
> So bad that in the ice driver (perhaps others too), we have cases that
> there is no counter or it has different meaning.
> (potentially "complicated" meaning - ice' struct
> ice_aqc_alloc_free_res_elem has "__le16 num_elems", so could not be used
> verbatim, and it's not actual counter either :/ (I was fooled by such

Speaking of __le16 (we already discussed it 1:1): it's not a rare case
to define Endianness-sensitive structures with a flex array at the end,
so for those with __{be,le}* we could be adding __counted_by() attribute
only when the host Endianness matches the structure's to have at least
some coverage. By "some" I mean actually a lot when it comes to LE
structures, which usually is the case :)

> assumption here [2]). Perhaps recent series by Olek [3] is also good
> illustration of hard cases for __counted_by()
> 
>>      int array[] __counted_by(count);
>> };

Thanks,
Olek