linux-kernel - Re: [RFC][PATCH] overflow: Twiddle with struct

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250306084015.GP5880@noisy.programming.kicks-ass.net>
Date: Thu, 6 Mar 2025 09:40:15 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Kees Cook <kees@...nel.org>
Cc: linux-kernel@...r.kernel.org, Borislav Petkov <bp@...en8.de>,
	linux-hardening@...r.kernel.org
Subject: Re: [RFC][PATCH] overflow: Twiddle with struct_size()

On Wed, Mar 05, 2025 at 11:13:00PM -0800, Kees Cook wrote:
> On Wed, Mar 05, 2025 at 02:43:15PM +0100, Peter Zijlstra wrote:
> > Hi Kees,
> > 
> > I keep getting hit by the struct_size() brigade, and I keep having
> > trouble reading that macro.
> > 
> > I had a wee poke and ended up with the below, WDYT?
> 
> Ah, and to clarify, this is just for readability? (There have been
> some tweaks to reduce the macro depths and other things in other areas,
> so I just wanted to check that wasn't, or was, part of the rationale.)

Yeah, the current thing nests that flex array macro thing, with the very
same arguments, which means you then have to go read that.

That combined with that nested constexptr toggle makes it a bit of a
mess.

> > (I also tried to create a __must_be_flex_array(), but utterly failed :/)
> 
> I spent a lot of time trying to find something for that too. :( If you
> do ever find it, please share! :)
> 
> > ---
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > index 0c7e3dcfe867..2123d0e238bb 100644
> > --- a/include/linux/overflow.h
> > +++ b/include/linux/overflow.h
> > @@ -352,9 +352,10 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
> >   * Return: number of bytes needed or SIZE_MAX on overflow.
> >   */
> >  #define flex_array_size(p, member, count)				\
> > -	__builtin_choose_expr(__is_constexpr(count),			\
> > -		(count) * sizeof(*(p)->member) + __must_be_array((p)->member),	\
> > -		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
> > +	(__must_be_array((p)->member) +					\
> > +	 __builtin_choose_expr(__is_constexpr(count),			\
> > +			       sizeof(*(p)->member) * (count),		\
> > +			       size_mul(sizeof(*(p)->member), (count))))
> 
> For both, I need to double check that __must_be_array() is
> always a constant expression. If not, we can't move it out of the
> __builtin_choose_expr(). But if so, then yeah, this is nice.

__must_be_array() is BUILD_BUG_ON() which must be a constant expression.

> >  
> >  /**
> >   * struct_size() - Calculate size of structure with trailing flexible array.
> > @@ -367,10 +368,12 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
> >   *
> >   * Return: number of bytes needed or SIZE_MAX on overflow.
> >   */
> > -#define struct_size(p, member, count)					\
> > -	__builtin_choose_expr(__is_constexpr(count),			\
> > -		sizeof(*(p)) + flex_array_size(p, member, count),	\
> > -		size_add(sizeof(*(p)), flex_array_size(p, member, count)))
> > +#define struct_size(p, member, count)					       \
> > +	(__must_be_array((p)->member) +					       \
> > +	 __builtin_choose_expr(__is_constexpr(count),			       \
> > +			       sizeof(*(p)) + (sizeof((p)->member) * (count)), \
> 
> typo: above should be                        (sizeof(*(p)->member))
> 
> (hint, to test this code use "./tools/testing/kunit/kunit.py run overflow")

D'oh. I sent the old version :-(. I actually did a kernel build and
fixed this error, and then went and sent the old one :/

> But yeah, this passes the overflow tests which include the constant
> expression tests, so __must_be_array() is a constant expression. Whee :)
> 
> > +			       size_add(sizeof(*(p)),			       \
> > +					size_mul(sizeof(*(p)->member), count))))
> 
> This one I'm not such a fan of. It feels wrong to not use
> flex_array_size() here -- we're performing exactly the same
> calculation. 

Yes, but now you can read it without also having to untangle
flex_array_size().

I did have a version using size_mad(x,y,z) := (x*y)+z, which is more
compact, but then you have the problem that you forever forget what
order around those 3 arguments are. (x*y)+z vs x+(y*z) etc.

Much like I can never remember the struct_size() argument order (at
least neovim's clangd lsp can somewhat help with that -- if I can get it
tamed).

> But it's possible this has cpp complexity reduction
> benefits from avoiding the stacking of __builtin_choose_expr() and
> __must_be_array() macros...

Just so.

> So, yeah, I think I could live with this. :) Especially if it means we can
> start using struct_size() in code you look at. :P
> 
> But please include this selftest update too. Since your patch splits the
> dependency between struct_size() and flex_array_size(), I'd like to test
> them separately now:

OK, will do. Thanks!