Re: [RESEND PATCH v2] string: uninline memcpy_and_pad
From: Guenter Roeck
Date: Wed Nov 03 2021 - 14:03:40 EST
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@xxxxxxxxxxxxxx/
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@xxxxxxxxxxxxxxxxxxxx>
Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Reviewed-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Acked-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
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