linux-kernel - Re: [PATCH] fortify: turn strlen() into an inline function using __builtin_constant

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <202501081331.E4B6DF747@keescook>
Date: Wed, 8 Jan 2025 13:46:50 -0800
From: Kees Cook <kees@...nel.org>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
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 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))


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

  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).

The lib/test_fortify/ target still successfully builds, so that's good
though. :)

-- 
Kees Cook