Message-ID: <CAMZ6RqJS5FwuBUdvVdXoZj6wZOJz8Nao4FzSzCK1VtbNF41sMw@mail.gmail.com>
Date: Thu, 9 Jan 2025 16:52:40 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Kees Cook <kees@...nel.org>
Cc: Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
	David Laight <david.laight@...lab.com>, linux-hardening@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] fortify: turn strlen() into an inline function using __builtin_constant_p()

On Thu. 9 Jan 2025 at 06:46, Kees Cook <kees@...nel.org> wrote:
> On Wed, Jan 08, 2025 at 11:27:51PM +0900, Vincent Mailhol wrote:
> > The strlen(p) function-like macro uses:
> >
> >   __is_constexpr(__builtin_strlen(p))
> >
> > in which GCC would only yield true if the argument p is a string
> > literal. Otherwise, GCC would return false even if p is a const
> > string.
> >
> > In contrary, by using:
> >
> >   __builtin_constant_p(__builtin_strlen(p))
> >
> > then GCC can also recognizes when p is a compile time constant string.
> >
> > The above is illustrated in [1].
> >
> > N.B.: clang is not impacted by any of this and gives the same results
> > with either __is_constexpr() and __builting_constant_p().
> >
> > Use __builtin_constant_p() instead of __is_constexpr() so that GCC can
> > do the folding on constant strings. This done, strlen() does not
> > require any more to be a function-like macro, so turn it into a static
> > inline function. In the process, __fortify_strlen() had to be moved
> > above strlen() so that it became visible to strlen().
>
> This is what __compiletime_strlen() ended up doing, so this seems
> reasonable to me.
>
> > On a side note, strlen() did a double expansion of its argument p.
>
> It did? Ah, was it due to __is_constexpr() wrapping? The other
> expressions should have been side-effect free:
>
>        __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)), \
>                __builtin_strlen(p), __fortify_strlen(p))

This does not have a side-effect. The issue is not the side effect but
the double expansion itself (actually here it is a triple expansion)
because it may grow exponentially. Linus explained this in greater
details here:

  https://lore.kernel.org/all/CAHk-=wjpN4GWtnsWQ8XJvf=gBQ3UvBk512xK1S35=nGXA6yTiw@mail.gmail.com/

Note that I do not believe this to be a big deal here. Unless the
strlen() macro gets called in others macro which themselves do double
parameter expansion, the issue should not surface. And honestly, I do
not see this happening with strings. That's why I put this as a side
note, it's more of a "follow a best practice" than solving an actual
problem.

On double thought, the risk does not seem real here. I will simply
drop this paragraph about double expansion in v2.

> I don't think you build-tested this with Clang, though?

I just did and...

  ./include/linux/fortify-string.h:272:17: error: redeclaration of
'strlen' must not have the 'overloadable' attribute
    272 | __kernel_size_t strlen(const char *p)
        |                 ^
  ./include/linux/string.h:200:24: note: previous unmarked overload of
function is here
    200 | extern __kernel_size_t strlen(const char *);
        |                        ^
  1 error generated.

OK, guilty here! Changing the function prototype to:

  __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1)
  __kernel_size_t strlen(const char * const POS p)

resolves the issue. I was wondering what this POS was for, guess I
have my answer.

>   CC      scripts/mod/devicetable-offsets.s
> In file included from ../scripts/mod/devicetable-offsets.c:3:
> In file included from ../include/linux/mod_devicetable.h:14:
> In file included from ../include/linux/uuid.h:11:
> In file included from ../include/linux/string.h:389:
> ../include/linux/fortify-string.h:272:17: error: redeclaration of 'strlen' must not have the 'overloadable' attribute
>   272 | __kernel_size_t strlen(const char *p)
>       |                 ^
> ../include/linux/string.h:200:24: note: previous unmarked overload of function is here
>   200 | extern __kernel_size_t strlen(const char *);
>       |                        ^
>
> The externs will need to be reworked if it's no longer depending on asm
> renaming.
>
> > Turning it into an inline function also resolved this side issue.
> >
> > [1] https://godbolt.org/z/rqr3YvoP4
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> > ---
> > This patch is the successor of patch [1] which was part of a longer
> > series [2]. Meanwhile, I decided to split it, so I am sending this again,
> > but as a stand-alone patch.
> >
> > Changelog since [1]: use __builtin_constant_p() instead and turn
> > strlen() into an inline function
> >
> > [1] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-6-4e4cbaecc216@wanadoo.fr/
> > [2] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-0-4e4cbaecc216@wanadoo.fr/
> > ---
> >  include/linux/fortify-string.h | 34 +++++++++++++++++++---------------
> >  1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index e4ce1cae03bf770047ce8a7c032b183683388cd5..bd22dd66e5f5b66ad839df42247e6436e0afd053 100644
> > --- a/include/linux/fortify-string.h
> > +++ b/include/linux/fortify-string.h
> > @@ -4,7 +4,6 @@
> >
> >  #include <linux/bitfield.h>
> >  #include <linux/bug.h>
> > -#include <linux/const.h>
> >  #include <linux/limits.h>
> >
> >  #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
> > @@ -241,6 +240,21 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
> >   * possible for strlen() to be used on compile-time strings for use in
> >   * static initializers (i.e. as a constant expression).
>
> ^^ This comment, however, is I think what sinks this patch. Please see
> commit 67ebc3ab4462 ("fortify: Make sure strlen() may still be used as a
> constant expression") which required that strlen() not be an inline. I'm
> pretty sure the build will start failing again (though perhaps only on
> older GCC versions).

Strange. I tested it with GCC 5.1 on godbolt and it worked fine. After
more investigation, I only got complains from GCC up to 4.9.4:

  https://godbolt.org/z/rW8r74vE3

I also just did a successful defconfig build using GCC 5.4 (sorry, I
do not have an environment with GCC 5.1).

So, it looks like an issue of the past to me. But in 67ebc3ab4462, the
minimum compiler version was already 5.1:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/min-tool-version.sh?id=67ebc3ab4462#n20

In the end, I am not sure what was the issue you encountered at that time.

Well, I am not able to reproduce, but if you tell me this is an issue,
then I can just keep the s/__is_constexpr/__builtin_constant_p/g
change in v2 and drop the inline function part (thus keeping the
triple expansion).

Let me know if you still think that having it as a function is a no
go. In the end, the main purpose here is to get rid of the
__is_constexpr. Turning the macro into a function still looks slightly
better to me, but at the end I do not really mind.


Yours sincerely,
Vincent Mailhol

Powered by blists - more mailing lists