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

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

[Kees Cook]

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");
+
+static __init int strlen_selftest(void)
+{
+	/* String length ruler:         123456789012345 */
+	static const char normal[]   = "I am normal";
+	static const char *ptr       = "where do I go?";
+	static const char trailing[] = "hidden NULLs\0\0\0";
+	static const char leading[]  = "\0\0hidden text";
+
+	if (strlen(normal) != 11)
+		return 0x100001;
+	if (strlen(ptr++) != 14)
+		return 0x100002;
+	if (strlen(ptr++) != 13)
+		return 0x100003;
+	if (strlen(trailing) != 12)
+		return 0x100004;
+	if (strlen(leading) != 0)
+		return 0x100005;
+	if (strlen_ce != 27)
+		return 0x100006;
+
+	return 0;
+}
+
 static __exit void string_selftest_remove(void)
 {
 }
@@ -212,6 +244,11 @@ static __init int string_selftest_init(void)
 	if (subtest)
 		goto fail;
 
+	test = 5;
+	subtest = strlen_selftest();
+	if (subtest)
+		goto fail;
+
 	pr_info("String selftests succeeded\n");
 	return 0;
 fail:
-- 
2.30.2

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

[Andy Shevchenko]

On Sun, Jan 30, 2022 at 8:36 PM Kees Cook <keescook@chromium.org> wrote:
>
> Add a simple test for strlen() functionality, including using it as a
> constant expression.

...

> +/*
> + * 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");

So, the compiler will replace this by a constant and then eliminate
the condition completely from the code. Did I understand this
correctly?

> +static __init int strlen_selftest(void)
> +{
> +       /* String length ruler:         123456789012345 */
> +       static const char normal[]   = "I am normal";
> +       static const char *ptr       = "where do I go?";
> +       static const char trailing[] = "hidden NULLs\0\0\0";
> +       static const char leading[]  = "\0\0hidden text";
> +
> +       if (strlen(normal) != 11)
> +               return 0x100001;
> +       if (strlen(ptr++) != 14)
> +               return 0x100002;
> +       if (strlen(ptr++) != 13)
> +               return 0x100003;
> +       if (strlen(trailing) != 12)
> +               return 0x100004;
> +       if (strlen(leading) != 0)
> +               return 0x100005;

> +       if (strlen_ce != 27)
> +               return 0x100006;

...so this part won't ever appear in the assembly (assuming -O2).

Same to the rest? If so, why is this not a part of the compiler tests?

> +       return 0;
> +}


-- 
With Best Regards,
Andy Shevchenko

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

[kernel test robot]

Hi Kees,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220131/202201310303.HQlCsvvd-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d22fe883339c5141953dbca980b51466ac3e2329
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
        git checkout d22fe883339c5141953dbca980b51466ac3e2329
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash M=lib/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> lib/test_string.c:188:30: error: initializer element is not constant
     188 | static const int strlen_ce = strlen("tada, a constant expression");
         |                              ^~~~~~


vim +188 lib/test_string.c

   181	
   182	/*
   183	 * Unlike many other string functions, strlen() can be used in
   184	 * static initializers when string lengths are known at compile
   185	 * time. (i.e. Under these conditions, strlen() is a constant
   186	 * expression.) Make sure it can be used this way.
   187	 */
 > 188	static const int strlen_ce = strlen("tada, a constant expression");
   189	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

Hi Kees,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220131/202201310303.HQlCsvvd-lkp(a)intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d22fe883339c5141953dbca980b51466ac3e2329
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
        git checkout d22fe883339c5141953dbca980b51466ac3e2329
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash M=lib/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> lib/test_string.c:188:30: error: initializer element is not constant
     188 | static const int strlen_ce = strlen("tada, a constant expression");
         |                              ^~~~~~


vim +188 lib/test_string.c

   181	
   182	/*
   183	 * Unlike many other string functions, strlen() can be used in
   184	 * static initializers when string lengths are known at compile
   185	 * time. (i.e. Under these conditions, strlen() is a constant
   186	 * expression.) Make sure it can be used this way.
   187	 */
 > 188	static const int strlen_ce = strlen("tada, a constant expression");
   189	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[Kees Cook]

On Sun, Jan 30, 2022 at 08:56:40PM +0200, Andy Shevchenko wrote:
> On Sun, Jan 30, 2022 at 8:36 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Add a simple test for strlen() functionality, including using it as a
> > constant expression.
> 
> ...
> 
> > +/*
> > + * 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");
> 
> So, the compiler will replace this by a constant and then eliminate
> the condition completely from the code. Did I understand this
> correctly?

Yup! See: https://godbolt.org/z/nTqPaszTh

There a few rare places in the kernel that do this, which is how
I noticed. (I broke strlen() with the recent FORTIFY changes.)

> > +static __init int strlen_selftest(void)
> > +{
> > +       /* String length ruler:         123456789012345 */
> > +       static const char normal[]   = "I am normal";
> > +       static const char *ptr       = "where do I go?";
> > +       static const char trailing[] = "hidden NULLs\0\0\0";
> > +       static const char leading[]  = "\0\0hidden text";
> > +
> > +       if (strlen(normal) != 11)
> > +               return 0x100001;
> > +       if (strlen(ptr++) != 14)
> > +               return 0x100002;
> > +       if (strlen(ptr++) != 13)
> > +               return 0x100003;
> > +       if (strlen(trailing) != 12)
> > +               return 0x100004;
> > +       if (strlen(leading) != 0)
> > +               return 0x100005;
> 
> > +       if (strlen_ce != 27)
> > +               return 0x100006;
> 
> ...so this part won't ever appear in the assembly (assuming -O2).

Correct, unless strlen() breaks.

> Same to the rest? If so, why is this not a part of the compiler tests?

I wanted to keep everything together -- this includes a macro
side-effect test as well ("ptr++").

-Kees

-- 
Kees Cook

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

[Kees Cook]

On Mon, Jan 31, 2022 at 04:13:30AM +0800, kernel test robot wrote:
> Hi Kees,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linux/master]
> [also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220128]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
> config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220131/202201310303.HQlCsvvd-lkp@intel.com/config)
> compiler: sh4-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/d22fe883339c5141953dbca980b51466ac3e2329
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
>         git checkout d22fe883339c5141953dbca980b51466ac3e2329
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash M=lib/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> >> lib/test_string.c:188:30: error: initializer element is not constant
>      188 | static const int strlen_ce = strlen("tada, a constant expression");
>          |                              ^~~~~~

Thanks; this is expected -- the strlen() in -next is currently broken
(and will be fixed in the next -next).

-Kees

> 
> 
> vim +188 lib/test_string.c
> 
>    181	
>    182	/*
>    183	 * Unlike many other string functions, strlen() can be used in
>    184	 * static initializers when string lengths are known at compile
>    185	 * time. (i.e. Under these conditions, strlen() is a constant
>    186	 * expression.) Make sure it can be used this way.
>    187	 */
>  > 188	static const int strlen_ce = strlen("tada, a constant expression");
>    189	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

-- 
Kees Cook

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

[Kees Cook]

[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]

On Mon, Jan 31, 2022 at 04:13:30AM +0800, kernel test robot wrote:
> Hi Kees,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linux/master]
> [also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220128]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
> config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220131/202201310303.HQlCsvvd-lkp(a)intel.com/config)
> compiler: sh4-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/d22fe883339c5141953dbca980b51466ac3e2329
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
>         git checkout d22fe883339c5141953dbca980b51466ac3e2329
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash M=lib/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> >> lib/test_string.c:188:30: error: initializer element is not constant
>      188 | static const int strlen_ce = strlen("tada, a constant expression");
>          |                              ^~~~~~

Thanks; this is expected -- the strlen() in -next is currently broken
(and will be fixed in the next -next).

-Kees

> 
> 
> vim +188 lib/test_string.c
> 
>    181	
>    182	/*
>    183	 * Unlike many other string functions, strlen() can be used in
>    184	 * static initializers when string lengths are known at compile
>    185	 * time. (i.e. Under these conditions, strlen() is a constant
>    186	 * expression.) Make sure it can be used this way.
>    187	 */
>  > 188	static const int strlen_ce = strlen("tada, a constant expression");
>    189	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

-- 
Kees Cook

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

[Guenter Roeck]

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.

Guenter

> +static __init int strlen_selftest(void)
> +{
> +	/* String length ruler:         123456789012345 */
> +	static const char normal[]   = "I am normal";
> +	static const char *ptr       = "where do I go?";
> +	static const char trailing[] = "hidden NULLs\0\0\0";
> +	static const char leading[]  = "\0\0hidden text";
> +
> +	if (strlen(normal) != 11)
> +		return 0x100001;
> +	if (strlen(ptr++) != 14)
> +		return 0x100002;
> +	if (strlen(ptr++) != 13)
> +		return 0x100003;
> +	if (strlen(trailing) != 12)
> +		return 0x100004;
> +	if (strlen(leading) != 0)
> +		return 0x100005;
> +	if (strlen_ce != 27)
> +		return 0x100006;
> +
> +	return 0;
> +}
> +
>  static __exit void string_selftest_remove(void)
>  {
>  }
> @@ -212,6 +244,11 @@ static __init int string_selftest_init(void)
>  	if (subtest)
>  		goto fail;
>  
> +	test = 5;
> +	subtest = strlen_selftest();
> +	if (subtest)
> +		goto fail;
> +
>  	pr_info("String selftests succeeded\n");
>  	return 0;
>  fail:
> -- 
> 2.30.2
> 

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

[Andy Shevchenko]

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:

...

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

With -O2 probably one can, otherwise I agree.

-- 
With Best Regards,
Andy Shevchenko

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

[Kees Cook]

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?

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),
};



> 
> Guenter
> 
> > +static __init int strlen_selftest(void)
> > +{
> > +	/* String length ruler:         123456789012345 */
> > +	static const char normal[]   = "I am normal";
> > +	static const char *ptr       = "where do I go?";
> > +	static const char trailing[] = "hidden NULLs\0\0\0";
> > +	static const char leading[]  = "\0\0hidden text";
> > +
> > +	if (strlen(normal) != 11)
> > +		return 0x100001;
> > +	if (strlen(ptr++) != 14)
> > +		return 0x100002;
> > +	if (strlen(ptr++) != 13)
> > +		return 0x100003;
> > +	if (strlen(trailing) != 12)
> > +		return 0x100004;
> > +	if (strlen(leading) != 0)
> > +		return 0x100005;
> > +	if (strlen_ce != 27)
> > +		return 0x100006;
> > +
> > +	return 0;
> > +}
> > +
> >  static __exit void string_selftest_remove(void)
> >  {
> >  }
> > @@ -212,6 +244,11 @@ static __init int string_selftest_init(void)
> >  	if (subtest)
> >  		goto fail;
> >  
> > +	test = 5;
> > +	subtest = strlen_selftest();
> > +	if (subtest)
> > +		goto fail;
> > +
> >  	pr_info("String selftests succeeded\n");
> >  	return 0;
> >  fail:
> > -- 
> > 2.30.2
> > 

-- 
Kees Cook

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

[Guenter Roeck]

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.

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.

Guenter

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

[Geert Uytterhoeven]

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.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Kees Cook]

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

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

[Kees Cook]

On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote:
> 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.

Hah, for i386:

arch/x86/Makefile
	# temporary until string.h is fixed
	KBUILD_CFLAGS += -ffreestanding

This "temporary" is from 2006. ;)

6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin")

Removing that appears to solve it, and appears to build correctly. I'll
continue testing.

> It works after dropping -ffreestanding.

I wonder if the other architectures were just copying x86?

-- 
Kees Cook

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

[Geert Uytterhoeven]

Hi Kees,

On Thu, Feb 3, 2022 at 6:15 PM Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote:
> > 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.
>
> Hah, for i386:
>
> arch/x86/Makefile
>         # temporary until string.h is fixed
>         KBUILD_CFLAGS += -ffreestanding
>
> This "temporary" is from 2006. ;)

And before that, we had it in the main Makefile.

> 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin")
>
> Removing that appears to solve it, and appears to build correctly. I'll
> continue testing.
>
> > It works after dropping -ffreestanding.
>
> I wonder if the other architectures were just copying x86?

At least on m68k it was added because gcc added new optimizations
that broke if an architecture provides some functions as inline.
As the kernel is not supposed to be linked with the standard C
library, -ffreestanding should be correct, isn't it?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 10:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Kees,
>
> On Thu, Feb 3, 2022 at 6:15 PM Kees Cook <keescook@chromium.org> wrote:
> > On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote:
> > > 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.
> >
> > Hah, for i386:
> >
> > arch/x86/Makefile
> >         # temporary until string.h is fixed
> >         KBUILD_CFLAGS += -ffreestanding
> >
> > This "temporary" is from 2006. ;)\

IIRC I sent a patch removing that. Yeah, Kees even signed off on it.
https://lore.kernel.org/lkml/20200817220212.338670-5-ndesaulniers@google.com/#t
I still think that's the right way to go, perhaps worth a resend to
rekick discussions.

>
> And before that, we had it in the main Makefile.
>
> > 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin")
> >
> > Removing that appears to solve it, and appears to build correctly. I'll
> > continue testing.
> >
> > > It works after dropping -ffreestanding.
> >
> > I wonder if the other architectures were just copying x86?
>
> At least on m68k it was added because gcc added new optimizations
> that broke if an architecture provides some functions as inline.
> As the kernel is not supposed to be linked with the standard C
> library, -ffreestanding should be correct, isn't it?

The kernel does not link against a libc; but it does provide many
symbols that libc would provide, with the same or similar enough
semantics that I would strongly recommend we _don't_ use
-ffreestanding in order to get such libcall optimizations (there are a
lot; see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
for some examples) and simply use -fno-builtin-* when necessary, or
fix the kernel implementations individually.

-- 
Thanks,
~Nick Desaulniers

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

[Kees Cook]

On Thu, Feb 03, 2022 at 11:50:34AM -0800, Nick Desaulniers wrote:
> On Thu, Feb 3, 2022 at 10:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Kees,
> >
> > On Thu, Feb 3, 2022 at 6:15 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote:
> > > > 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.
> > >
> > > Hah, for i386:
> > >
> > > arch/x86/Makefile
> > >         # temporary until string.h is fixed
> > >         KBUILD_CFLAGS += -ffreestanding
> > >
> > > This "temporary" is from 2006. ;)\
> 
> IIRC I sent a patch removing that. Yeah, Kees even signed off on it.
> https://lore.kernel.org/lkml/20200817220212.338670-5-ndesaulniers@google.com/#t
> I still think that's the right way to go, perhaps worth a resend to
> rekick discussions.

Hah. Yay. I'll go pick this into the memcpy topic branch.  Building x86_64
and ia32 differently makes no sense (and this solves the head-scratching
compile-time test failures I was seeing on ia32 too).

> >
> > And before that, we had it in the main Makefile.
> >
> > > 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin")
> > >
> > > Removing that appears to solve it, and appears to build correctly. I'll
> > > continue testing.
> > >
> > > > It works after dropping -ffreestanding.
> > >
> > > I wonder if the other architectures were just copying x86?
> >
> > At least on m68k it was added because gcc added new optimizations
> > that broke if an architecture provides some functions as inline.
> > As the kernel is not supposed to be linked with the standard C
> > library, -ffreestanding should be correct, isn't it?
> 
> The kernel does not link against a libc; but it does provide many
> symbols that libc would provide, with the same or similar enough
> semantics that I would strongly recommend we _don't_ use
> -ffreestanding in order to get such libcall optimizations (there are a
> lot; see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
> for some examples) and simply use -fno-builtin-* when necessary, or
> fix the kernel implementations individually.

Right, so, I think for x86 it's straight forward. For the other
architectures it may need more careful checking, so I'm just going
to drop the new test from the memcpy topic branch, and maybe start a
"-ffreestanding removal" topic branch so there isn't a cross-dependency
here.

-- 
Kees Cook

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

[Kees Cook]

On Thu, Feb 03, 2022 at 11:50:34AM -0800, Nick Desaulniers wrote:
> The kernel does not link against a libc; but it does provide many
> symbols that libc would provide, with the same or similar enough
> semantics that I would strongly recommend we _don't_ use
> -ffreestanding in order to get such libcall optimizations (there are a
> lot; see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
> for some examples) and simply use -fno-builtin-* when necessary, or
> fix the kernel implementations individually.

Right, we really don't want -ffreestanding. Rather, we want to not link
against libgcc. This is mostly true already, though some of the smaller
architectures still do:

$ git grep print-libgcc
arch/arc/Makefile:LIBGCC        = $(shell $(CC) $(cflags-y) --print-libgcc-file-name)
arch/csky/Makefile:     $(shell $(CC) $(KBUILD_CFLAGS) $(KCFLAGS) -print-libgcc-file-name)
arch/h8300/boot/compressed/Makefile:LIBGCC := $(shell $(CROSS-COMPILE)$(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name 2>/dev/null)
arch/nios2/Makefile:LIBGCC         := $(shell $(CC) $(KBUILD_CFLAGS) $(KCFLAGS) -print-libgcc-file-name)
arch/openrisc/Makefile:LIBGCC           := $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)
arch/parisc/Makefile:LIBGCC             := $(shell $(CC) -print-libgcc-file-name)
arch/xtensa/Makefile:LIBGCC := $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)
arch/xtensa/boot/boot-redboot/Makefile:LIBGCC := $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)

-- 
Kees Cook