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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAMZ6RqJbinVpsGf6ADUtQTYPRRQN=D8Ne_n6pHtDUPXxED5ZvQ@mail.gmail.com>
Date: Sat, 11 Jan 2025 23:40:41 +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 16:52, Vincent Mailhol <mailhol.vincent@...adoo.fr> wrote:
(...)
> > >   * 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.

Actually, I did more investigation and it is working for some strange
reasons. Whenever the argument of a function named strlen() is a
compile time constant, the compiler (both GCC and clang) will replace
it with the string length on the argument, even if strlen() is
programmed to return something else:

  https://godbolt.org/z/nK4b3fnM7

So it is only working because the compiler uses its builtin strlen()
instead of the function we provided.

Actually, we could just do:

  extern inline size_t strlen(const char *p)
  {
          return __fortify_strlen(p);
  }

and it would work: the compiler ignores the content whenever p is a
compile time constant ¯\_(ツ)_/¯

Well, all this is really ugly. I will drop the inline function and
send a v2 just with the s/__is_constexpr/__builtin_constant_p/g
replacement.


Yours sincerely,
Vincent Mailhol