From: Arnd Bergmann <[email protected]>
We are close to being able to turn on -Wstringop-truncation
unconditionally instead of only at the 'make W=1' level, these five
warnings remain after the previous round and three patches I sent
separately for drivers/staging.
I hope I managed to include all the feedback on v1, so please apply
directly to subsystem trees if v2 looks ok to you.
Arnd
Arnd Bergmann (5):
[v2] test_hexdump: avoid string truncation warning
[v2] acpi: disable -Wstringop-truncation
[v2] block/partitions/ldm: convert strncpy() to strscpy()
[v2] blktrace: convert strncpy() to strscpy_pad()
[v2] kbuild: enable -Wstringop-truncation globally
block/partitions/ldm.c | 6 ++----
drivers/acpi/acpica/Makefile | 1 +
kernel/trace/blktrace.c | 3 +--
lib/test_hexdump.c | 2 +-
scripts/Makefile.extrawarn | 1 -
5 files changed, 5 insertions(+), 8 deletions(-)
--
2.39.2
Cc: "Richard Russon" <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nicolas Schier <[email protected]>
Cc: Lin Ming <[email protected]>
Cc: Alexey Starikovskiy <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
From: Arnd Bergmann <[email protected]>
gcc can warn when a string is too long to fit into the strncpy()
destination buffer, as it is here depending on the function
arguments:
inlined from 'test_hexdump_prepare_test.constprop' at /home/arnd/arm-soc/lib/test_hexdump.c:116:3:
include/linux/fortify-string.h:108:33: error: '__builtin_strncpy' output truncated copying between 0 and 32 bytes from a string of length 32 [-Werror=stringop-truncation]
108 | #define __underlying_strncpy __builtin_strncpy
| ^
include/linux/fortify-string.h:187:16: note: in expansion of macro '__underlying_strncpy'
187 | return __underlying_strncpy(p, q, size);
| ^~~~~~~~~~~~~~~~~~~~
The intention here is to copy exactly 'l' bytes without any padding or
NUL-termination, so the most logical change is to use memcpy(), just as
a previous change adapted the other output from strncpy() to memcpy().
Cc: Justin Stitt <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
---
lib/test_hexdump.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index b916801f23a8..fe2682bb21e6 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -113,7 +113,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
*p++ = ' ';
} while (p < test + rs * 2 + rs / gs + 1);
- strncpy(p, data_a, l);
+ memcpy(p, data_a, l);
p += l;
}
--
2.39.2
From: Arnd Bergmann <[email protected]>
The strncpy() here can cause a non-terminated string, which older gcc
versions such as gcc-9 warn about:
In function 'ldm_parse_tocblock',
inlined from 'ldm_validate_tocblocks' at block/partitions/ldm.c:386:7,
inlined from 'ldm_partition' at block/partitions/ldm.c:1457:7:
block/partitions/ldm.c:134:2: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
134 | strncpy (toc->bitmap1_name, data + 0x24, sizeof (toc->bitmap1_name));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
block/partitions/ldm.c:145:2: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
145 | strncpy (toc->bitmap2_name, data + 0x46, sizeof (toc->bitmap2_name));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
New versions notice that the code is correct after all because of the
following termination, but replacing the strncpy() with strscpy_pad()
or strcpy() avoids the warning and simplifies the code at the same time.
Use the padding version here to keep the existing behavior, in case
the code relies on not including uninitialized data.
Reviewed-by: Justin Stitt <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
block/partitions/ldm.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/block/partitions/ldm.c b/block/partitions/ldm.c
index 38e58960ae03..2bd42fedb907 100644
--- a/block/partitions/ldm.c
+++ b/block/partitions/ldm.c
@@ -131,8 +131,7 @@ static bool ldm_parse_tocblock (const u8 *data, struct tocblock *toc)
ldm_crit ("Cannot find TOCBLOCK, database may be corrupt.");
return false;
}
- strncpy (toc->bitmap1_name, data + 0x24, sizeof (toc->bitmap1_name));
- toc->bitmap1_name[sizeof (toc->bitmap1_name) - 1] = 0;
+ strscpy_pad(toc->bitmap1_name, data + 0x24, sizeof(toc->bitmap1_name));
toc->bitmap1_start = get_unaligned_be64(data + 0x2E);
toc->bitmap1_size = get_unaligned_be64(data + 0x36);
@@ -142,8 +141,7 @@ static bool ldm_parse_tocblock (const u8 *data, struct tocblock *toc)
TOC_BITMAP1, toc->bitmap1_name);
return false;
}
- strncpy (toc->bitmap2_name, data + 0x46, sizeof (toc->bitmap2_name));
- toc->bitmap2_name[sizeof (toc->bitmap2_name) - 1] = 0;
+ strscpy_pad(toc->bitmap2_name, data + 0x46, sizeof(toc->bitmap2_name));
toc->bitmap2_start = get_unaligned_be64(data + 0x50);
toc->bitmap2_size = get_unaligned_be64(data + 0x58);
if (strncmp (toc->bitmap2_name, TOC_BITMAP2,
--
2.39.2
From: Arnd Bergmann <[email protected]>
The remaining warnings of this type have been addressed, so it can
now be enabled by default, rather than only for W=1.
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: no changes
---
scripts/Makefile.extrawarn | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 57edc80661fd..f4d69092698b 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -107,7 +107,6 @@ else
KBUILD_CFLAGS += $(call cc-disable-warning, format-overflow-non-kprintf)
KBUILD_CFLAGS += $(call cc-disable-warning, format-truncation-non-kprintf)
endif
-KBUILD_CFLAGS += $(call cc-disable-warning, stringop-truncation)
KBUILD_CFLAGS += -Wno-override-init # alias for -Wno-initializer-overrides in clang
--
2.39.2
From: Arnd Bergmann <[email protected]>
gcc-9 warns about a possibly non-terminated string copy:
kernel/trace/blktrace.c: In function 'do_blk_trace_setup':
kernel/trace/blktrace.c:527:2: error: 'strncpy' specified bound 32 equals destination size [-Werror=stringop-truncation]
Newer versions are fine here because they see the following explicit
nul-termination. Using strscpy_pad() avoids the warning and
simplifies the code a little. The padding helps give a clean
buffer to userspace.
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: actually use padding version of strscpy.
---
kernel/trace/blktrace.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index d5d94510afd3..8fd292d34d89 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -524,8 +524,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
if (!buts->buf_size || !buts->buf_nr)
return -EINVAL;
- strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
- buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
+ strscpy_pad(buts->name, name, BLKTRACE_BDEV_SIZE);
/*
* some device names have larger paths - convert the slashes
--
2.39.2
From: Arnd Bergmann <[email protected]>
gcc -Wstringop-truncation warns about copying a string that results in a
missing nul termination:
drivers/acpi/acpica/tbfind.c: In function 'acpi_tb_find_table':
drivers/acpi/acpica/tbfind.c:60:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
60 | strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/acpi/acpica/tbfind.c:61:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
61 | strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The code works as intended, and the warning could be addressed by using
a memcpy(), but turning the warning off for this file works equally well
and may be easir to merge.
Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
Link: https://lore.kernel.org/lkml/CAJZ5v0hoUfv54KW7y4223Mn9E7D4xvR7whRFNLTBqCZMUxT50Q@mail.gmail.com/#t
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/acpi/acpica/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile
index 30f3fc13c29d..8d18af396de9 100644
--- a/drivers/acpi/acpica/Makefile
+++ b/drivers/acpi/acpica/Makefile
@@ -5,6 +5,7 @@
ccflags-y := -D_LINUX -DBUILDING_ACPICA
ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT
+CFLAGS_tbfind.o += $(call cc-disable-warning, stringop-truncation)
# use acpi.o to put all files here into acpi.o modparam namespace
obj-y += acpi.o
--
2.39.2
On Tue, Apr 9, 2024 at 4:01 PM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> gcc -Wstringop-truncation warns about copying a string that results in a
> missing nul termination:
>
> drivers/acpi/acpica/tbfind.c: In function 'acpi_tb_find_table':
> drivers/acpi/acpica/tbfind.c:60:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
> 60 | strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/acpi/acpica/tbfind.c:61:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
> 61 | strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The code works as intended, and the warning could be addressed by using
> a memcpy(), but turning the warning off for this file works equally well
> and may be easir to merge.
"easier" (fixed up).
Tricky that, but OK.let's get the warning off the table.
Applied as 6.10 material, thanks!
> Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
> Link: https://lore.kernel.org/lkml/CAJZ5v0hoUfv54KW7y4223Mn9E7D4xvR7whRFNLTBqCZMUxT50Q@mail.gmail.com/#t
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/acpi/acpica/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile
> index 30f3fc13c29d..8d18af396de9 100644
> --- a/drivers/acpi/acpica/Makefile
> +++ b/drivers/acpi/acpica/Makefile
> @@ -5,6 +5,7 @@
>
> ccflags-y := -D_LINUX -DBUILDING_ACPICA
> ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT
> +CFLAGS_tbfind.o += $(call cc-disable-warning, stringop-truncation)
>
> # use acpi.o to put all files here into acpi.o modparam namespace
> obj-y += acpi.o
> --
> 2.39.2
>
>
On Tue, Apr 09, 2024 at 04:00:54PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc can warn when a string is too long to fit into the strncpy()
> destination buffer, as it is here depending on the function
> arguments:
>
> inlined from 'test_hexdump_prepare_test.constprop' at /home/arnd/arm-soc/lib/test_hexdump.c:116:3:
> include/linux/fortify-string.h:108:33: error: '__builtin_strncpy' output truncated copying between 0 and 32 bytes from a string of length 32 [-Werror=stringop-truncation]
> 108 | #define __underlying_strncpy __builtin_strncpy
> | ^
> include/linux/fortify-string.h:187:16: note: in expansion of macro '__underlying_strncpy'
> 187 | return __underlying_strncpy(p, q, size);
> | ^~~~~~~~~~~~~~~~~~~~
>
> The intention here is to copy exactly 'l' bytes without any padding or
> NUL-termination, so the most logical change is to use memcpy(), just as
> a previous change adapted the other output from strncpy() to memcpy().
>
> Cc: Justin Stitt <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
This looks better than the previous strscpy() usage.
Acked-by: Justin Stitt <[email protected]>
> ---
> ---
> lib/test_hexdump.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index b916801f23a8..fe2682bb21e6 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -113,7 +113,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
> *p++ = ' ';
> } while (p < test + rs * 2 + rs / gs + 1);
>
> - strncpy(p, data_a, l);
> + memcpy(p, data_a, l);
> p += l;
> }
>
> --
> 2.39.2
>
Hi,
On Tue, Apr 09, 2024 at 04:00:55PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc -Wstringop-truncation warns about copying a string that results in a
> missing nul termination:
>
> drivers/acpi/acpica/tbfind.c: In function 'acpi_tb_find_table':
> drivers/acpi/acpica/tbfind.c:60:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
> 60 | strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/acpi/acpica/tbfind.c:61:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
> 61 | strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The code works as intended, and the warning could be addressed by using
> a memcpy(), but turning the warning off for this file works equally well
> and may be easir to merge.
Dang, I would've really liked to see these strncpy()'s dealt with [1]!
The warning is there because that specific usage of strncpy is plain
wrong. strncpy() is a string api and this usage looks like it has
arguments and results not resembling C-strings.
Not sure if turning off correct warnings is the right call.
Link: https://github.com/KSPP/linux/issues/90 [1]
>
> Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
> Link: https://lore.kernel.org/lkml/CAJZ5v0hoUfv54KW7y4223Mn9E7D4xvR7whRFNLTBqCZMUxT50Q@mail.gmail.com/#t
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/acpi/acpica/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile
> index 30f3fc13c29d..8d18af396de9 100644
> --- a/drivers/acpi/acpica/Makefile
> +++ b/drivers/acpi/acpica/Makefile
> @@ -5,6 +5,7 @@
>
> ccflags-y := -D_LINUX -DBUILDING_ACPICA
> ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT
> +CFLAGS_tbfind.o += $(call cc-disable-warning, stringop-truncation)
>
> # use acpi.o to put all files here into acpi.o modparam namespace
> obj-y += acpi.o
> --
> 2.39.2
>
Thanks
Justin
Hi,
On Tue, Apr 09, 2024 at 04:00:57PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc-9 warns about a possibly non-terminated string copy:
>
> kernel/trace/blktrace.c: In function 'do_blk_trace_setup':
> kernel/trace/blktrace.c:527:2: error: 'strncpy' specified bound 32 equals destination size [-Werror=stringop-truncation]
>
> Newer versions are fine here because they see the following explicit
> nul-termination. Using strscpy_pad() avoids the warning and
> simplifies the code a little. The padding helps give a clean
> buffer to userspace.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Acked-by: Justin Stitt <[email protected]>
> ---
> v2: actually use padding version of strscpy.
> ---
> kernel/trace/blktrace.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index d5d94510afd3..8fd292d34d89 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -524,8 +524,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
> if (!buts->buf_size || !buts->buf_nr)
> return -EINVAL;
>
> - strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
> - buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
> + strscpy_pad(buts->name, name, BLKTRACE_BDEV_SIZE);
>
> /*
> * some device names have larger paths - convert the slashes
> --
> 2.39.2
>
Thanks
Justin