[PATCH v3 1/3] string: Make stpcpy() possible to use

[PATCH v3 1/3] string: Make stpcpy() possible to use

[Andy Shevchenko]

It is a good rule to avoid submitting code without users.
Currently the stpcpy() is unusable due to missed declaration.
Any attempts to use it will bring something like:

  error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]

Move declaration to the header and guard it as other string functions.

Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
Reported-by: kernel test robot <lkp@intel.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: new patch to fix reported issue
 include/linux/string.h | 3 +++
 lib/string.c           | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b6572aeca2f5..b1aeb3475396 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
 #ifndef __HAVE_ARCH_STRSCPY
 ssize_t strscpy(char *, const char *, size_t);
 #endif
+#ifndef __HAVE_ARCH_STPCPY
+char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
+#endif
 
 /* Wraps calls to strscpy()/memset(), no arch specific code required */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count);
diff --git a/lib/string.c b/lib/string.c
index 485777c9da83..4ecb8ec1fdd1 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -233,6 +233,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
 EXPORT_SYMBOL(strscpy);
 #endif
 
+#ifndef __HAVE_ARCH_STPCPY
 /**
  * stpcpy - copy a string from src to dest returning a pointer to the new end
  *          of dest, including src's %NUL-terminator. May overrun dest.
@@ -248,7 +249,6 @@ EXPORT_SYMBOL(strscpy);
  * not recommended for usage. Instead, its definition is provided in case
  * the compiler lowers other libcalls to stpcpy.
  */
-char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
 char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
 {
 	while ((*dest++ = *src++) != '\0')
@@ -256,6 +256,7 @@ char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
 	return --dest;
 }
 EXPORT_SYMBOL(stpcpy);
+#endif
 
 #ifndef __HAVE_ARCH_STRCAT
 /**
-- 
2.34.1

[PATCH v3 2/3] vsprintf: Fix potential unaligned access

[Andy Shevchenko]

The %p4cc specifier in some cases might get an unaligned pointer.
Due to this we need to make copy to local variable once to avoid
potential crashes on some architectures due to improper access.

Fixes: af612e43de6d ("lib/vsprintf: Add support for printing V4L2 and DRM fourccs")
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
v3: no changes
 lib/vsprintf.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 61528094ec87..4e8f3e9acb99 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -49,6 +49,7 @@
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
+#include <asm/unaligned.h>
 
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
@@ -1762,7 +1763,7 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	char output[sizeof("0123 little-endian (0x01234567)")];
 	char *p = output;
 	unsigned int i;
-	u32 val;
+	u32 orig, val;
 
 	if (fmt[1] != 'c' || fmt[2] != 'c')
 		return error_string(buf, end, "(%p4?)", spec);
@@ -1770,21 +1771,22 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	if (check_pointer(&buf, end, fourcc, spec))
 		return buf;
 
-	val = *fourcc & ~BIT(31);
+	orig = get_unaligned(fourcc);
+	val = orig & ~BIT(31);
 
-	for (i = 0; i < sizeof(*fourcc); i++) {
+	for (i = 0; i < sizeof(u32); i++) {
 		unsigned char c = val >> (i * 8);
 
 		/* Print non-control ASCII characters as-is, dot otherwise */
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
 	p += strlen(p);
 
 	*p++ = ' ';
 	*p++ = '(';
-	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
+	p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
 	*p++ = ')';
 	*p = '\0';
 
-- 
2.34.1

[PATCH v3 3/3] vsprintf: Move space out of string literals in fourcc_string()

[Andy Shevchenko]

The literals "big-endian" and "little-endian" may be potentially
occurred in other places. Dropping space allows linker to
"compress" them by using only a single copy.

Rasmus suggested, while at it, replacing strcpy() + strlen() by
p = stpcpy(), which is done here as well.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
v3: no changes
 lib/vsprintf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 4e8f3e9acb99..e2a1d89f1a5c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
-	p += strlen(p);
+	*p++ = ' ';
+	p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
 
 	*p++ = ' ';
 	*p++ = '(';
-- 
2.34.1

Re: [PATCH v3 3/3] vsprintf: Move space out of string literals in fourcc_string()

[Kees Cook]

On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:
> The literals "big-endian" and "little-endian" may be potentially
> occurred in other places. Dropping space allows linker to
> "compress" them by using only a single copy.
> 
> Rasmus suggested, while at it, replacing strcpy() + strlen() by
> p = stpcpy(), which is done here as well.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> v3: no changes
>  lib/vsprintf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 4e8f3e9acb99..e2a1d89f1a5c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  		*p++ = isascii(c) && isprint(c) ? c : '.';
>  	}
>  
> -	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
> -	p += strlen(p);
> +	*p++ = ' ';
> +	p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
>  
>  	*p++ = ' ';
>  	*p++ = '(';

No need for any of the manual construction nor stpcpy():

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7ad44f2c8f5..aef8bd2789a9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1788,14 +1788,9 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
-	p += strlen(p);
-
-	*p++ = ' ';
-	*p++ = '(';
-	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
-	*p++ = ')';
-	*p = '\0';
+	snprintf(p, sizeof(output) - sizeof(*fourcc), " %s (0x%x)",
+		*fourcc & BIT(31) ? "big-endian" : " little-endian",
+		*fourcc);
 
 	return string(buf, end, output, spec);
 }


(untested)

-- 
Kees Cook

Re: [PATCH v3 3/3] vsprintf: Move space out of string literals in fourcc_string()

[Sakari Ailus]

Hi Kees,

On Wed, Jan 26, 2022 at 01:22:32PM -0800, Kees Cook wrote:
> On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:
> > The literals "big-endian" and "little-endian" may be potentially
> > occurred in other places. Dropping space allows linker to
> > "compress" them by using only a single copy.
> > 
> > Rasmus suggested, while at it, replacing strcpy() + strlen() by
> > p = stpcpy(), which is done here as well.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > v3: no changes
> >  lib/vsprintf.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 4e8f3e9acb99..e2a1d89f1a5c 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> >  		*p++ = isascii(c) && isprint(c) ? c : '.';
> >  	}
> >  
> > -	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
> > -	p += strlen(p);
> > +	*p++ = ' ';
> > +	p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> >  
> >  	*p++ = ' ';
> >  	*p++ = '(';
> 
> No need for any of the manual construction nor stpcpy():
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7ad44f2c8f5..aef8bd2789a9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1788,14 +1788,9 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  		*p++ = isascii(c) && isprint(c) ? c : '.';
>  	}
>  
> -	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> -	p += strlen(p);
> -
> -	*p++ = ' ';
> -	*p++ = '(';
> -	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
> -	*p++ = ')';
> -	*p = '\0';
> +	snprintf(p, sizeof(output) - sizeof(*fourcc), " %s (0x%x)",
> +		*fourcc & BIT(31) ? "big-endian" : " little-endian",
> +		*fourcc);
>  
>  	return string(buf, end, output, spec);
>  }
> 
> 
> (untested)

This could work but results in two nested calls to vsnprintf(). I'd
definitely avoid that, due to stack usage and then albeit it's easy to see
the recursion will end with two iterations, that will very fast cease to be
the case if you keep applying the pattern.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 3/3] vsprintf: Move space out of string literals in fourcc_string()

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 01:22:32PM -0800, Kees Cook wrote:
> On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:

...

> >  	*p++ = ' ';
> >  	*p++ = '(';
> 
> No need for any of the manual construction nor stpcpy():

No, we are better to avoid recursive printf() calls.
Entire vsprintf.c is written keeping that in mind.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] string: Make stpcpy() possible to use

[Nathan Chancellor]

On Wed, Jan 26, 2022 at 04:19:15PM +0200, Andy Shevchenko wrote:
> It is a good rule to avoid submitting code without users.

While I agree with the sentiment in the general case, I don't think that
it applies in this case and this comment should be dropped. The message
of the commit this fixes and the comment right above the declaration
both make it pretty obvious why this interface was added with no in-tree
users and why the declaration was placed right above the definition.

> Currently the stpcpy() is unusable due to missed declaration.
> Any attempts to use it will bring something like:
> 
>   error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> 
> Move declaration to the header and guard it as other string functions.
> 
> Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Regardless, the commit itself seems fine from a technical standpoint. I
won't comment on whether or not this interface should be opened up.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
> v3: new patch to fix reported issue
>  include/linux/string.h | 3 +++
>  lib/string.c           | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..b1aeb3475396 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
>  #ifndef __HAVE_ARCH_STRSCPY
>  ssize_t strscpy(char *, const char *, size_t);
>  #endif
> +#ifndef __HAVE_ARCH_STPCPY
> +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
> +#endif
>  
>  /* Wraps calls to strscpy()/memset(), no arch specific code required */
>  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> diff --git a/lib/string.c b/lib/string.c
> index 485777c9da83..4ecb8ec1fdd1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -233,6 +233,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
>  EXPORT_SYMBOL(strscpy);
>  #endif
>  
> +#ifndef __HAVE_ARCH_STPCPY
>  /**
>   * stpcpy - copy a string from src to dest returning a pointer to the new end
>   *          of dest, including src's %NUL-terminator. May overrun dest.
> @@ -248,7 +249,6 @@ EXPORT_SYMBOL(strscpy);
>   * not recommended for usage. Instead, its definition is provided in case
>   * the compiler lowers other libcalls to stpcpy.
>   */
> -char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
>  char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
>  {
>  	while ((*dest++ = *src++) != '\0')
> @@ -256,6 +256,7 @@ char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
>  	return --dest;
>  }
>  EXPORT_SYMBOL(stpcpy);
> +#endif
>  
>  #ifndef __HAVE_ARCH_STRCAT
>  /**
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 1/3] string: Make stpcpy() possible to use

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 09:38:02AM -0700, Nathan Chancellor wrote:
> On Wed, Jan 26, 2022 at 04:19:15PM +0200, Andy Shevchenko wrote:
> > It is a good rule to avoid submitting code without users.
> 
> While I agree with the sentiment in the general case, I don't think that
> it applies in this case and this comment should be dropped. The message
> of the commit this fixes and the comment right above the declaration
> both make it pretty obvious why this interface was added with no in-tree
> users and why the declaration was placed right above the definition.

Thanks for accenting on this. Yes, I see now the reasoning and I don't
know which way is better. As a consumer of this API it shows a room for
micro-optimizations (I dunno if GCC and/or Clang able to replace the two
by stpcpy(), as done in the patch, at compile time).

That said, depending on the others' opinions let see how to proceed.

> > Currently the stpcpy() is unusable due to missed declaration.
> > Any attempts to use it will bring something like:
> > 
> >   error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> > 
> > Move declaration to the header and guard it as other string functions.
> > 
> > Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Nathan Chancellor <natechancellor@gmail.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Regardless, the commit itself seems fine from a technical standpoint. I
> won't comment on whether or not this interface should be opened up.
> 
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] string: Make stpcpy() possible to use

[Nick Desaulniers]

On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> It is a good rule to avoid submitting code without users.
> Currently the stpcpy() is unusable due to missed declaration.
> Any attempts to use it will bring something like:
>
>   error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
>
> Move declaration to the header and guard it as other string functions.
>
> Fixes: 1e1b6d63d634 ("lib/string.c: implement stpcpy")
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Recall the discussion from Kees:
https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
and
https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/

I'm guessing that the 0day bot report was on the third patch in this
series, which looks to use stpcpy:
https://lore.kernel.org/lkml/20220126141917.75399-3-andriy.shevchenko@linux.intel.com/

for patch 3, I'd s/"compress"/de-duplicate/ or s/"compress"/merge/.

Kees, I'm curious if we can do something like require the string
length to be known at compile time otherwise BUILD_BUG_ON?

> ---
> v3: new patch to fix reported issue
>  include/linux/string.h | 3 +++
>  lib/string.c           | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..b1aeb3475396 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -31,6 +31,9 @@ size_t strlcpy(char *, const char *, size_t);
>  #ifndef __HAVE_ARCH_STRSCPY
>  ssize_t strscpy(char *, const char *, size_t);
>  #endif
> +#ifndef __HAVE_ARCH_STPCPY
> +char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
> +#endif
>
>  /* Wraps calls to strscpy()/memset(), no arch specific code required */
>  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> diff --git a/lib/string.c b/lib/string.c
> index 485777c9da83..4ecb8ec1fdd1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -233,6 +233,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
>  EXPORT_SYMBOL(strscpy);
>  #endif
>
> +#ifndef __HAVE_ARCH_STPCPY
>  /**
>   * stpcpy - copy a string from src to dest returning a pointer to the new end
>   *          of dest, including src's %NUL-terminator. May overrun dest.
> @@ -248,7 +249,6 @@ EXPORT_SYMBOL(strscpy);
>   * not recommended for usage. Instead, its definition is provided in case
>   * the compiler lowers other libcalls to stpcpy.
>   */
> -char *stpcpy(char *__restrict__ dest, const char *__restrict__ src);
>  char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
>  {
>         while ((*dest++ = *src++) != '\0')
> @@ -256,6 +256,7 @@ char *stpcpy(char *__restrict__ dest, const char *__restrict__ src)
>         return --dest;
>  }
>  EXPORT_SYMBOL(stpcpy);
> +#endif
>
>  #ifndef __HAVE_ARCH_STRCAT
>  /**
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v3 1/3] string: Make stpcpy() possible to use

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 09:49:38AM -0800, Nick Desaulniers wrote:
> On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > It is a good rule to avoid submitting code without users.
> > Currently the stpcpy() is unusable due to missed declaration.
> > Any attempts to use it will bring something like:
> >
> >   error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> >
> > Move declaration to the header and guard it as other string functions.

...

> Recall the discussion from Kees:
> https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
> and
> https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/

> I'm guessing that the 0day bot report was on the third patch in this
> series, which looks to use stpcpy:
> https://lore.kernel.org/lkml/20220126141917.75399-3-andriy.shevchenko@linux.intel.com/
> 
> for patch 3,

Yes. It was Rasmus' suggestion.

> I'd s/"compress"/de-duplicate/ or s/"compress"/merge/.

I'll fix it locally.

> Kees, I'm curious if we can do something like require the string
> length to be known at compile time otherwise BUILD_BUG_ON?

Perhaps static_assert() is better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] string: Make stpcpy() possible to use

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 09:49:38AM -0800, Nick Desaulniers wrote:
> On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > It is a good rule to avoid submitting code without users.
> > Currently the stpcpy() is unusable due to missed declaration.
> > Any attempts to use it will bring something like:
> >
> >   error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> >
> > Move declaration to the header and guard it as other string functions.

...

> Recall the discussion from Kees:
> https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
> and
> https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/

For the record :-)
https://lore.kernel.org/lkml/CAHp75VfniSw3AFTyyDk2OoAChGx7S6wF7sZKpJXNHmk97BoRXA@mail.gmail.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] string: Make stpcpy() possible to use

[Kees Cook]

On Wed, Jan 26, 2022 at 08:12:22PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 09:49:38AM -0800, Nick Desaulniers wrote:
> > On Wed, Jan 26, 2022 at 6:19 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > It is a good rule to avoid submitting code without users.
> > > Currently the stpcpy() is unusable due to missed declaration.
> > > Any attempts to use it will bring something like:
> > >
> > >   error: implicit declaration of function ‘stpcpy’ [-Werror=implicit-function-declaration]
> > >
> > > Move declaration to the header and guard it as other string functions.
> 
> ...
> 
> > Recall the discussion from Kees:
> > https://lore.kernel.org/lkml/CAK7LNAQXo5-5W6hvNMEVPBPf3tRWaf-pQdSR-0OHyi4RCGhjsQ@mail.gmail.com/
> > and
> > https://lore.kernel.org/lkml/202008150921.B70721A359@keescook/
> 
> For the record :-)
> https://lore.kernel.org/lkml/CAHp75VfniSw3AFTyyDk2OoAChGx7S6wF7sZKpJXNHmk97BoRXA@mail.gmail.com/
> [...
> strcpy() is not a bad API for the cases when you know what you are
> doing. A problem that most of the developers do not know what they are
> doing.
> No need to split everything to bad and good by its name or semantics,
> each API has its own pros and cons and programmers must use their
> brains.
> ...]

Developers should not need to remember to avoid foot-guns; the toolchain
should be doing all of that. The trouble is that C (and its standard
libs) are filled with foot-guns.

I do not want to add another foot-gun API to the kernel; we've been
working very hard to _remove_ them. :) If the kernel's stpcpy() _only_
worked on all known-size strings, etc, so that memory safety could be
determined at compile-time, then I'd have no objection.

What's not clear to me is if such macro versions would be workable for
the reason stpcpy() was added in the first place, which was the compiler
transforming other calls stuff into library calls it thinks are defined.

Totally untested:

#define stpcpy(dst, src) ({					\
	size_t _stp__dst = __builtin_object_size(dst, 1);	\
	size_t _stp__src = __builtin_object_size(src, 1);	\
								\
	BUILD_BUG_ON(_stp__dst == -1 || _stp__src == -1);	\
	BUILD_BUG_ON(_stp__src > _stp__dst);			\
								\
	__builtin_stpcpy(dst, src);				\
})

(Is there even a __builtin_stpcpy()?)

-- 
Kees Cook