[RESEND PATCH v2] string: uninline memcpy_and_pad

[RESEND PATCH v2] string: uninline memcpy_and_pad

[Guenter Roeck]

When building m68k:allmodconfig, recent versions of gcc generate the
following error if the length of UTS_RELEASE is less than 8 bytes.

In function 'memcpy_and_pad',
    inlined from 'nvmet_execute_disc_identify' at
    	drivers/nvme/target/discovery.c:268:2:
arch/m68k/include/asm/string.h:72:25: error:
	'__builtin_memcpy' reading 8 bytes from a region of size 7

Discussions around the problem suggest that this only happens if an
architecture does not provide strlen(), if -ffreestanding is provided as
compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
for m68k. The exact reasons are unknown, but seem to be related to the
ability of the compiler to evaluate the return value of strlen() and
the resulting execution flow in memcpy_and_pad(). It would be possible
to work around the problem by using sizeof(UTS_RELEASE) instead of
strlen(UTS_RELEASE), but that would only postpone the problem until the
function is called in a similar way. Uninline memcpy_and_pad() instead
to solve the problem for good.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Moved to lib/string_helpers.c
    Balanced { } in if/else statement to make checkpatch happy
    Added Reviewed-by: /Acked-by: tags

 include/linux/string.h | 19 ++-----------------
 lib/string_helpers.c   | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 5a36608144a9..b6572aeca2f5 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -253,23 +253,8 @@ static inline const char *kbasename(const char *path)
 #include <linux/fortify-string.h>
 #endif
 
-/**
- * memcpy_and_pad - Copy one buffer to another with padding
- * @dest: Where to copy to
- * @dest_len: The destination buffer size
- * @src: Where to copy from
- * @count: The number of bytes to copy
- * @pad: Character to use for padding if space is left in destination.
- */
-static inline void memcpy_and_pad(void *dest, size_t dest_len,
-				  const void *src, size_t count, int pad)
-{
-	if (dest_len > count) {
-		memcpy(dest, src, count);
-		memset(dest + count, pad,  dest_len - count);
-	} else
-		memcpy(dest, src, dest_len);
-}
+void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
+		    int pad);
 
 /**
  * memset_after - Set a value after a struct member to the end of a struct
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index faa9d8e4e2c5..d5d008f5b1d9 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -883,6 +883,26 @@ char *strreplace(char *s, char old, char new)
 }
 EXPORT_SYMBOL(strreplace);
 
+/**
+ * memcpy_and_pad - Copy one buffer to another with padding
+ * @dest: Where to copy to
+ * @dest_len: The destination buffer size
+ * @src: Where to copy from
+ * @count: The number of bytes to copy
+ * @pad: Character to use for padding if space is left in destination.
+ */
+void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
+		    int pad)
+{
+	if (dest_len > count) {
+		memcpy(dest, src, count);
+		memset(dest + count, pad,  dest_len - count);
+	} else {
+		memcpy(dest, src, dest_len);
+	}
+}
+EXPORT_SYMBOL(memcpy_and_pad);
+
 #ifdef CONFIG_FORTIFY_SOURCE
 void fortify_panic(const char *name)
 {
-- 
2.33.0

Re: [RESEND PATCH v2] string: uninline memcpy_and_pad

[Kees Cook]

On Tue, Nov 02, 2021 at 07:25:18AM -0700, Guenter Roeck wrote:
> When building m68k:allmodconfig, recent versions of gcc generate the
> following error if the length of UTS_RELEASE is less than 8 bytes.
> 
> In function 'memcpy_and_pad',
>     inlined from 'nvmet_execute_disc_identify' at
>     	drivers/nvme/target/discovery.c:268:2:
> arch/m68k/include/asm/string.h:72:25: error:
> 	'__builtin_memcpy' reading 8 bytes from a region of size 7

What things are size 8 and 7? "5.15.0\0" is 7 bytes -- is strlen()
returning _8_?? It should return 6 here.

> Discussions around the problem suggest that this only happens if an
> architecture does not provide strlen(), if -ffreestanding is provided as
> compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
> for m68k. The exact reasons are unknown, but seem to be related to the
> ability of the compiler to evaluate the return value of strlen() and
> the resulting execution flow in memcpy_and_pad(). It would be possible
> to work around the problem by using sizeof(UTS_RELEASE) instead of
> strlen(UTS_RELEASE), but that would only postpone the problem until the
> function is called in a similar way. Uninline memcpy_and_pad() instead
> to solve the problem for good.

Ew, no, this doesn't solve the problem -- it just makes the buffer sizes
invisible to the compiler. This will hide legitimate problems. Please
leave this inlined.

struct nvme_id_ctrl {
	...
        char                    fr[8];
...

        memcpy_and_pad(id->fr, sizeof(id->fr),
                       UTS_RELEASE, strlen(UTS_RELEASE), ' ');

	memcpy_and_pad(id->fr, 8, UTS_RELEASE, 6, ' ')


static inline void memcpy_and_pad(void *dest, size_t dest_len,
				  const void *src, size_t count, int pad)
	if (dest_len > count) {
		memcpy(dest, src, count);
		memset(dest + count, pad,  dest_len - count);
	} else {
		memcpy(dest, src, dest_len);
	}

->

	if (8 > 6) {
		memcpy(id->fr, UTS_RELEASE, 6);
		memset(fd->fr + 6, ' ', 2);
	}

Ah, I've found the earlier thread now:
https://lore.kernel.org/all/CAMuHMdX365qmWiii=gQLADpW49EMkdDrVJDPWNBpAZuZM0WQFQ@mail.gmail.com/

This looks like a compiler bug in that it isn't collapsing the strlen()
into a compile-time constant. "sizeof(UTS_RELEASE) - 1" seems a
reasonable work-around?

I'd suggest reporting this to GCC. I see two bugs:

- strlen() not getting replace with a constant expression
- warning being generated across a runtime check

it seems like GCC *thinks* it knows strlen will be a CE, but then
instead it keeps both branches anyway, triggering a warning.

-Kees

> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Moved to lib/string_helpers.c
>     Balanced { } in if/else statement to make checkpatch happy
>     Added Reviewed-by: /Acked-by: tags
> 
>  include/linux/string.h | 19 ++-----------------
>  lib/string_helpers.c   | 20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 5a36608144a9..b6572aeca2f5 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -253,23 +253,8 @@ static inline const char *kbasename(const char *path)
>  #include <linux/fortify-string.h>
>  #endif
>  
> -/**
> - * memcpy_and_pad - Copy one buffer to another with padding
> - * @dest: Where to copy to
> - * @dest_len: The destination buffer size
> - * @src: Where to copy from
> - * @count: The number of bytes to copy
> - * @pad: Character to use for padding if space is left in destination.
> - */
> -static inline void memcpy_and_pad(void *dest, size_t dest_len,
> -				  const void *src, size_t count, int pad)
> -{
> -	if (dest_len > count) {
> -		memcpy(dest, src, count);
> -		memset(dest + count, pad,  dest_len - count);
> -	} else
> -		memcpy(dest, src, dest_len);
> -}
> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> +		    int pad);
>  
>  /**
>   * memset_after - Set a value after a struct member to the end of a struct
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index faa9d8e4e2c5..d5d008f5b1d9 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -883,6 +883,26 @@ char *strreplace(char *s, char old, char new)
>  }
>  EXPORT_SYMBOL(strreplace);
>  
> +/**
> + * memcpy_and_pad - Copy one buffer to another with padding
> + * @dest: Where to copy to
> + * @dest_len: The destination buffer size
> + * @src: Where to copy from
> + * @count: The number of bytes to copy
> + * @pad: Character to use for padding if space is left in destination.
> + */
> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> +		    int pad)
> +{
> +	if (dest_len > count) {
> +		memcpy(dest, src, count);
> +		memset(dest + count, pad,  dest_len - count);
> +	} else {
> +		memcpy(dest, src, dest_len);
> +	}
> +}
> +EXPORT_SYMBOL(memcpy_and_pad);
> +
>  #ifdef CONFIG_FORTIFY_SOURCE
>  void fortify_panic(const char *name)
>  {
> -- 
> 2.33.0
> 

-- 
Kees Cook

Re: [RESEND PATCH v2] string: uninline memcpy_and_pad

[Guenter Roeck]

On 11/3/21 10:37 AM, Kees Cook wrote:
> On Tue, Nov 02, 2021 at 07:25:18AM -0700, Guenter Roeck wrote:
>> When building m68k:allmodconfig, recent versions of gcc generate the
>> following error if the length of UTS_RELEASE is less than 8 bytes.
>>
>> In function 'memcpy_and_pad',
>>      inlined from 'nvmet_execute_disc_identify' at
>>      	drivers/nvme/target/discovery.c:268:2:
>> arch/m68k/include/asm/string.h:72:25: error:
>> 	'__builtin_memcpy' reading 8 bytes from a region of size 7
> 
> What things are size 8 and 7? "5.15.0\0" is 7 bytes -- is strlen()
> returning _8_?? It should return 6 here.
> 
>> Discussions around the problem suggest that this only happens if an
>> architecture does not provide strlen(), if -ffreestanding is provided as
>> compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case
>> for m68k. The exact reasons are unknown, but seem to be related to the
>> ability of the compiler to evaluate the return value of strlen() and
>> the resulting execution flow in memcpy_and_pad(). It would be possible
>> to work around the problem by using sizeof(UTS_RELEASE) instead of
>> strlen(UTS_RELEASE), but that would only postpone the problem until the
>> function is called in a similar way. Uninline memcpy_and_pad() instead
>> to solve the problem for good.
> 
> Ew, no, this doesn't solve the problem -- it just makes the buffer sizes
> invisible to the compiler. This will hide legitimate problems. Please
> leave this inlined.
> 
> struct nvme_id_ctrl {
> 	...
>          char                    fr[8];
> ...
> 
>          memcpy_and_pad(id->fr, sizeof(id->fr),
>                         UTS_RELEASE, strlen(UTS_RELEASE), ' ');
> 
> 	memcpy_and_pad(id->fr, 8, UTS_RELEASE, 6, ' ')
> 
> 
> static inline void memcpy_and_pad(void *dest, size_t dest_len,
> 				  const void *src, size_t count, int pad)
> 	if (dest_len > count) {
> 		memcpy(dest, src, count);
> 		memset(dest + count, pad,  dest_len - count);
> 	} else {
> 		memcpy(dest, src, dest_len);
> 	}
> 
> ->
> 
> 	if (8 > 6) {
> 		memcpy(id->fr, UTS_RELEASE, 6);
> 		memset(fd->fr + 6, ' ', 2);
> 	}
> 
> Ah, I've found the earlier thread now:
> https://lore.kernel.org/all/CAMuHMdX365qmWiii=gQLADpW49EMkdDrVJDPWNBpAZuZM0WQFQ@mail.gmail.com/
> 
> This looks like a compiler bug in that it isn't collapsing the strlen()
> into a compile-time constant. "sizeof(UTS_RELEASE) - 1" seems a
> reasonable work-around?
> 
> I'd suggest reporting this to GCC. I see two bugs:
> 
> - strlen() not getting replace with a constant expression

I do not think this can be classified as compiler bug. The compiler
does not know the result of strlen() because the version from
lib/string.c is used. It does know the length of UTS_RELEASE,
and it does know the length of fd->fr. Since it doesn't know
the return value of strlen(), it concludes that the else path
in memcpy_and_pad() may be taken, and comes to the wrong conclusion.

> - warning being generated across a runtime check
> 
I am not sure about that one either. gcc knows the size of dest,
the value of dest_len, and the size of src. I don't know what kind
of conclusions a compiler may make based on that information.

The problem I see is that if any kind of flow ahead of memcpy()
with compile-time constant parameters invalidates checks the compiler
can make based on those parameters, a lot of similar checks which
_are_ possible today may be disabled. Declaring this a compiler bug
and getting it "fixed" may have the undesirable side effect of
disabling a lot of useful checks. Maybe that is not a concern, but
I think it is something to be aware of.

> it seems like GCC *thinks* it knows strlen will be a CE, but then
> instead it keeps both branches anyway, triggering a warning.
> 

I don't think it believes that strlen is a CE.

Either case, I'll wait for advice from Linus on how to proceed.

Thanks,
Guenter

> -Kees
> 
>>
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Moved to lib/string_helpers.c
>>      Balanced { } in if/else statement to make checkpatch happy
>>      Added Reviewed-by: /Acked-by: tags
>>
>>   include/linux/string.h | 19 ++-----------------
>>   lib/string_helpers.c   | 20 ++++++++++++++++++++
>>   2 files changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 5a36608144a9..b6572aeca2f5 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -253,23 +253,8 @@ static inline const char *kbasename(const char *path)
>>   #include <linux/fortify-string.h>
>>   #endif
>>   
>> -/**
>> - * memcpy_and_pad - Copy one buffer to another with padding
>> - * @dest: Where to copy to
>> - * @dest_len: The destination buffer size
>> - * @src: Where to copy from
>> - * @count: The number of bytes to copy
>> - * @pad: Character to use for padding if space is left in destination.
>> - */
>> -static inline void memcpy_and_pad(void *dest, size_t dest_len,
>> -				  const void *src, size_t count, int pad)
>> -{
>> -	if (dest_len > count) {
>> -		memcpy(dest, src, count);
>> -		memset(dest + count, pad,  dest_len - count);
>> -	} else
>> -		memcpy(dest, src, dest_len);
>> -}
>> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>> +		    int pad);
>>   
>>   /**
>>    * memset_after - Set a value after a struct member to the end of a struct
>> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
>> index faa9d8e4e2c5..d5d008f5b1d9 100644
>> --- a/lib/string_helpers.c
>> +++ b/lib/string_helpers.c
>> @@ -883,6 +883,26 @@ char *strreplace(char *s, char old, char new)
>>   }
>>   EXPORT_SYMBOL(strreplace);
>>   
>> +/**
>> + * memcpy_and_pad - Copy one buffer to another with padding
>> + * @dest: Where to copy to
>> + * @dest_len: The destination buffer size
>> + * @src: Where to copy from
>> + * @count: The number of bytes to copy
>> + * @pad: Character to use for padding if space is left in destination.
>> + */
>> +void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>> +		    int pad)
>> +{
>> +	if (dest_len > count) {
>> +		memcpy(dest, src, count);
>> +		memset(dest + count, pad,  dest_len - count);
>> +	} else {
>> +		memcpy(dest, src, dest_len);
>> +	}
>> +}
>> +EXPORT_SYMBOL(memcpy_and_pad);
>> +
>>   #ifdef CONFIG_FORTIFY_SOURCE
>>   void fortify_panic(const char *name)
>>   {
>> -- 
>> 2.33.0
>>
> 

Re: [RESEND PATCH v2] string: uninline memcpy_and_pad

[Linus Torvalds]

On Wed, Nov 3, 2021 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Either case, I'll wait for advice from Linus on how to proceed.

It's a completely unimportant function, used in a couple of places,
not performance-sensitive, not security-sensitive, and not worth any
kind of buffer size checking.

And it's breaking the build because being inline somehow confuses the compiler.

It gets out-of-lined.

Kees - once you have gotten all compilers fixed and everybody using
them, you can send a patch to make it inline again.

I estimate that is in about ten years or so.

             Linus

Re: [RESEND PATCH v2] string: uninline memcpy_and_pad

[Kees Cook]

On Wed, Nov 03, 2021 at 11:34:05AM -0700, Linus Torvalds wrote:
> Kees - once you have gotten all compilers fixed and everybody using
> them, you can send a patch to make it inline again.

Fair enough. I've got a pile of changes underway that do touch a bunch
of these things, so yeah, we'll have pretty wide coverage soon enough.

> I estimate that is in about ten years or so.

Heh. Well, luckily, both GCC and Clang have been pretty responsive about
getting legitimate bugs[1] fixed, so I'm gonna aim for "less than 10
years". ;)

-Kees

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101419

-- 
Kees Cook