lkml.org 
[lkml]   [2021]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RESEND PATCH v2] string: uninline memcpy_and_pad
Date
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
>>
>

\
 
 \ /
  Last update: 2021-11-03 19:05    [W:0.114 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site