Re: [PATCH] lib/test_string.c: Add test for strlen()

[Kees Cook <keescook@chromium.org>]

On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> On Thu, Feb 3, 2022 at 12:12 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > On 2/2/22 12:52, Kees Cook wrote:
> > > On Wed, Feb 02, 2022 at 08:01:49AM -0800, Guenter Roeck wrote:
> > >> On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote:
> > >>> Add a simple test for strlen() functionality, including using it as a
> > >>> constant expression.
> > >>>
> > >>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > >>> Cc: Peter Rosin <peda@axentia.se>
> > >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> > >>> ---
> > >>> I'll be taking this as part of my Clang FORTIFY_SOURCE series.
> > >>> ---
> > >>>   lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++
> > >>>   1 file changed, 37 insertions(+)
> > >>>
> > >>> diff --git a/lib/test_string.c b/lib/test_string.c
> > >>> index 9dfd6f52de92..59994f552c48 100644
> > >>> --- a/lib/test_string.c
> > >>> +++ b/lib/test_string.c
> > >>> @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void)
> > >>>     return 0;
> > >>>   }
> > >>>
> > >>> +/*
> > >>> + * Unlike many other string functions, strlen() can be used in
> > >>> + * static initializers when string lengths are known at compile
> > >>> + * time. (i.e. Under these conditions, strlen() is a constant
> > >>> + * expression.) Make sure it can be used this way.
> > >>> + */
> > >>> +static const int strlen_ce = strlen("tada, a constant expression");
> > >>> +
> > >>
> > >> This results in:
> > >>
> > >> lib/test_string.c:188:30: error: initializer element is not constant
> > >>    188 | static const int strlen_ce = strlen("tada, a constant expression");
> > >>
> > >> for several of my tests. I don't think you can mandate that a compiler
> > >> implements this.
> > >
> > > Which tests?
> > >
> >
> > Some examples:
> >
> > Build reference: next-20220202
> > Compiler version: m68k-linux-gcc (GCC) 11.2.0
> >
> > Building m68k:defconfig ... failed
> > --------------
> > Error log:
> > lib/test_string.c:188:30: error: initializer element is not constant
> >    188 | static const int strlen_ce = strlen("tada, a constant expression");
> >
> > Building mips:malta_defconfig:nocd:smp:net,e1000:initrd ... failed
> > ------------
> > Error log:
> > lib/test_string.c:188:30: error: initializer element is not constant
> >   static const int strlen_ce = strlen("tada, a constant expression");
> >
> > Building i386:q35:Broadwell:defconfig:smp:ata:net,rtl8139:hd ... failed
> > ------------
> > Error log:
> > lib/test_string.c:188:30: error: initializer element is not constant
> >    188 | static const int strlen_ce = strlen("tada, a constant expression");
> >
> > i386 and is defconfig + CONFIG_STRING_SELFTEST=y; mips is
> > malta_defconfig + CONFIG_STRING_SELFTEST=y. All use gcc 11.2.
> >
> > There may be more, but there are so many failures in -next right now
> > that I may be missing some.
> >
> > > This property of strlen() is already required by our builds (this is how
> > > I tripped over it). For example:
> > >
> > > drivers/firmware/xilinx/zynqmp-debug.c:
> > >
> > > #define PM_API(id)               {id, #id, strlen(#id)}
> > > static struct pm_api_info pm_api_list[] = {
> > >          PM_API(PM_GET_API_VERSION),
> > >          PM_API(PM_QUERY_DATA),
> > > };
> >
> > I do not think that it is a C standard that strlen() on a constant string
> > must be compile-time evaluated and result in a constant.
> 
> Not if -ffreestanding, which is what several architectures are
> using nowadays, to a.o. prevent gcc from replacing calls to stdlib
> functions to other stdlib functions (e.g. strncat() -> strlen() +
> store, strncmp() -> strcmp()), which breaks linking if the latter is
> only provided inline.
> 
> > Anyway, key difference, I think, is the presence of an architecture-specific
> > version of strlen(), or the maybe non-presence of __HAVE_ARCH_STRLEN,
> > or the definition of strlen() in include/linux/fortify-string.h.
> 
> It works after dropping -ffreestanding.

Ah-ha, thanks for the clue here. I'll see if there is some way to
detect this (or, I guess, drop the patch). I don't like that this
requirement depends on the architecture, so I'd rather it behave the
same everywhere. Hmm

-- 
Kees Cook