netdev - Re: [PATCH net-next v3 1/7] overflow: add DEFINE

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]

Date: Fri, 18 Aug 2023 12:28:15 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: David Laight <David.Laight@...LAB.COM>, 'Kees Cook'
	<keescook@...omium.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Jacob Keller
	<jacob.e.keller@...el.com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, "linux-hardening@...r.kernel.org"
	<linux-hardening@...r.kernel.org>, Steven Zou <steven.zou@...el.com>
Subject: Re: [PATCH net-next v3 1/7] overflow: add DEFINE_FLEX() for on-stack
 allocs

On 8/18/23 09:14, David Laight wrote:
> From: Kees Cook
>> Sent: Thursday, August 17, 2023 6:00 PM
>>
>> On Thu, Aug 17, 2023 at 02:35:23PM +0000, David Laight wrote:
>>> From: Przemek Kitszel
>>>> Sent: Wednesday, August 16, 2023 3:06 PM
> ...
>>>> +#define DEFINE_FLEX(type, name, member, count)					\
>>>> +	union {									\
>>>> +		u8 bytes[struct_size_t(type, member, count)];			\
>>>> +		type obj;							\
>>>> +	} name##_u __aligned(_Alignof(type)) = {};				\
>>>
>>> You shouldn't need the _Alignof() it is the default.
>>
>> In the sense that since "type" is in the union, it's okay?
> 
> The alignment of the union is the larger of the alignments
> of all its members.
> Which is what you want.
> 
>>> I'm not sure you should be forcing the memset() either.
>>
>> This already got discussed: better to fail safe.
> 
> Perhaps call it DEFINE_FLEX_Z() to make this clear and
> give the option for a non-zeroing version later.
> Not everyone wants the expense of zeroing everything.

per Kees, zeroing should be removed by compiler when not needed:
https://lore.kernel.org/intel-wired-lan/202308101128.C4F0FA235@keescook/

Thanks for education on alignment!

> 
> ..
>>> You might want to add:
>>> 	Static_assert(is_constexpr(count), "DEFINE_FLEX: non-constant count " #count);
>>
>> That would be nice, though can Static_assert()s live in the middle of
>> variable definitions?
> 
> I checked and it is fine.
> (I double-checked by adding a statement and getting an error.)

Static_assert with nice wording definitively would make it better, 
thanks again!

> 
> 	David >
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 
>