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 ten
warnings are all that I saw in randconfig testing across compiler versions
on arm, arm64 and x86.
The final patch is only there for reference at the moment, I hope
we can merge the other ones through the subsystem trees first,
as there are no dependencies between them.
Arnd
Arnd Bergmann (11):
staging: vc04_services: changen strncpy() to strscpy_pad()
scsi: devinfo: rework scsi_strcpy_devinfo()
staging: replace weird strncpy() with memcpy()
orangefs: convert strncpy() to strscpy()
test_hexdump: avoid string truncation warning
acpi: avoid warning for truncated string copy
block/partitions/ldm: convert strncpy() to strscpy()
blktrace: convert strncpy() to strscpy_pad()
staging: rtl8723bs: convert strncpy to strscpy
staging: greybus: change strncpy() to strscpy()
kbuild: enable -Wstringop-truncation globally
block/partitions/ldm.c | 6 ++--
drivers/acpi/acpica/tbfind.c | 19 +++++------
drivers/scsi/scsi_devinfo.c | 30 +++++++++++------
drivers/staging/greybus/fw-management.c | 4 +--
.../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 5 ++-
drivers/staging/rts5208/rtsx_scsi.c | 2 +-
.../vc04_services/vchiq-mmal/mmal-vchiq.c | 4 +--
fs/orangefs/dcache.c | 4 +--
fs/orangefs/namei.c | 33 +++++++++----------
fs/orangefs/super.c | 16 ++++-----
kernel/trace/blktrace.c | 3 +-
lib/test_hexdump.c | 2 +-
scripts/Makefile.extrawarn | 1 -
13 files changed, 64 insertions(+), 65 deletions(-)
--
2.39.2
Cc: "Richard Russon
Cc: Jens Axboe <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: Viresh Kumar <[email protected]>
Cc: Johan Hovold <[email protected]>
Cc: Alex Elder <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Broadcom internal kernel review list <[email protected]>
Cc: Mike Marshall <[email protected]>
Cc: Martin Brandenburg <[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: Arnd Bergmann <[email protected]>
Cc: Kees Cook <[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]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
From: Arnd Bergmann <[email protected]>
gcc-14 warns about this strncpy() that results in a non-terminated
string for an overflow:
In file included from include/linux/string.h:369,
from drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:20:
In function 'strncpy',
inlined from 'create_component' at drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:940:2:
include/linux/fortify-string.h:108:33: error: '__builtin_strncpy' specified bound 128 equals destination size [-Werror=stringop-truncation]
Change it to strscpy_pad(), which produces a properly terminated and
zero-padded string.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index 258aa0e37f55..6ca5797aeae5 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -937,8 +937,8 @@ static int create_component(struct vchiq_mmal_instance *instance,
/* build component create message */
m.h.type = MMAL_MSG_TYPE_COMPONENT_CREATE;
m.u.component_create.client_component = component->client_component;
- strncpy(m.u.component_create.name, name,
- sizeof(m.u.component_create.name));
+ strscpy_pad(m.u.component_create.name, name,
+ sizeof(m.u.component_create.name));
ret = send_synchronous_mmal_msg(instance, &m,
sizeof(m.u.component_create),
--
2.39.2
From: Arnd Bergmann <[email protected]>
scsi_strcpy_devinfo() appears to work as intended but its semantics are
so confusing that gcc warns about it when -Wstringop-truncation is enabled:
In function 'scsi_strcpy_devinfo',
inlined from 'scsi_dev_info_list_add_keyed' at drivers/scsi/scsi_devinfo.c:370:2:
drivers/scsi/scsi_devinfo.c:297:9: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
297 | strncpy(to, from, to_length);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Reorganize the function to completely separate the nul-terminated from
the space-padded/non-terminated case. The former is just strscpy_pad(),
while the latter does not have a standard function.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/scsi/scsi_devinfo.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index ba7237e83863..58726c15ebac 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -290,18 +290,28 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key)
static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
char *from, int compatible)
{
- size_t from_length;
+ int ret;
- from_length = strlen(from);
- /* This zero-pads the destination */
- strncpy(to, from, to_length);
- if (from_length < to_length && !compatible) {
- /*
- * space pad the string if it is short.
- */
- memset(&to[from_length], ' ', to_length - from_length);
+ if (compatible) {
+ /* This zero-pads and nul-terminates the destination */
+ ret = strscpy_pad(to, from, to_length);
+ } else {
+ /* no nul-termination but space-padding for short strings */
+ size_t from_length = strlen(from);
+ ret = from_length;
+
+ if (from_length > to_length) {
+ from_length = to_length;
+ ret = -E2BIG;
+ }
+
+ memcpy(to, from, from_length);
+
+ if (from_length < to_length)
+ memset(&to[from_length], ' ', to_length - from_length);
}
- if (from_length > to_length)
+
+ if (ret < 0)
printk(KERN_WARNING "%s: %s string '%s' is too long\n",
__func__, name, from);
}
--
2.39.2
From: Arnd Bergmann <[email protected]>
When -Wstringop-truncation is enabled, gcc finds a function that
always does a short copy:
In function 'inquiry',
inlined from 'rtsx_scsi_handler' at drivers/staging/rts5208/rtsx_scsi.c:3210:12:
drivers/staging/rts5208/rtsx_scsi.c:526:17: error: 'strncpy' output truncated copying between 1 and 28 bytes from a string of length 28 [-Werror=stringop-truncation]
526 | strncpy(buf + 8, inquiry_string, sendbytes - 8);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Since the actual size of the copy is already known at this point, just
copy the bytes directly and skip the length check and zero-padding.
This partially reverts an earlier bugfix that replaced the original
incorrect memcpy() with a less bad strncpy(), but it now also avoids
the original overflow.
Fixes: 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/staging/rts5208/rtsx_scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
index 08bd768ad34d..a73b0959f5a9 100644
--- a/drivers/staging/rts5208/rtsx_scsi.c
+++ b/drivers/staging/rts5208/rtsx_scsi.c
@@ -523,7 +523,7 @@ static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
if (sendbytes > 8) {
memcpy(buf, inquiry_buf, 8);
- strncpy(buf + 8, inquiry_string, sendbytes - 8);
+ memcpy(buf + 8, inquiry_string, min(sendbytes, 36) - 8);
if (pro_formatter_flag) {
/* Additional Length */
buf[4] = 0x33;
--
2.39.2
From: Arnd Bergmann <[email protected]>
gcc warns about a truncated string copy with a 255 byte string getting
copied to a buffer of the same length, losing the 0-termination:
In function 'orangefs_unmount',
inlined from 'orangefs_kill_sb' at arm-soc/fs/orangefs/super.c:619:6:
fs/orangefs/super.c:406:9: error: 'strncpy' output may be truncated copying 255 bytes from a string of length 255 [-Werror=stringop-truncation]
406 | strncpy(op->upcall.req.fs_umount.orangefs_config_server,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
407 | devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I see that most string copies in orangefs are for the upcalls and use
a buffer that is one short to get the implied termination from the
zero-filled buffer, but some other instances lack the '-1' part.
Convert from strncpy() to strscpy() to avoids both the warning about
the buffer size and the need for the explicit padding, since strscpy
guarantees a zero-terminated buffer.
Signed-off-by: Arnd Bergmann <[email protected]>
---
fs/orangefs/dcache.c | 4 ++--
fs/orangefs/namei.c | 33 +++++++++++++++------------------
fs/orangefs/super.c | 16 +++++++---------
3 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index 8bbe9486e3a6..96ed9900f7a9 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -33,9 +33,9 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
new_op->upcall.req.lookup.parent_refn = parent->refn;
- strncpy(new_op->upcall.req.lookup.d_name,
+ strscpy(new_op->upcall.req.lookup.d_name,
dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ ORANGEFS_NAME_MAX);
gossip_debug(GOSSIP_DCACHE_DEBUG,
"%s:%s:%d interrupt flag [%d]\n",
diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
index c9dfd5c6a097..5e46d3bdcb05 100644
--- a/fs/orangefs/namei.c
+++ b/fs/orangefs/namei.c
@@ -41,8 +41,8 @@ static int orangefs_create(struct mnt_idmap *idmap,
fill_default_sys_attrs(new_op->upcall.req.create.attributes,
ORANGEFS_TYPE_METAFILE, mode);
- strncpy(new_op->upcall.req.create.d_name,
- dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.create.d_name,
+ dentry->d_name.name, ORANGEFS_NAME_MAX);
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
@@ -137,8 +137,8 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
&parent->refn.khandle);
new_op->upcall.req.lookup.parent_refn = parent->refn;
- strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
+ ORANGEFS_NAME_MAX);
gossip_debug(GOSSIP_NAME_DEBUG,
"%s: doing lookup on %s under %pU,%d\n",
@@ -192,8 +192,8 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry)
return -ENOMEM;
new_op->upcall.req.remove.parent_refn = parent->refn;
- strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
+ ORANGEFS_NAME_MAX);
ret = service_operation(new_op, "orangefs_unlink",
get_interruptible_flag(inode));
@@ -247,10 +247,9 @@ static int orangefs_symlink(struct mnt_idmap *idmap,
ORANGEFS_TYPE_SYMLINK,
mode);
- strncpy(new_op->upcall.req.sym.entry_name,
- dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
- strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.sym.entry_name,
+ dentry->d_name.name, ORANGEFS_NAME_MAX);
+ strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX);
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
@@ -324,8 +323,8 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes,
ORANGEFS_TYPE_DIRECTORY, mode);
- strncpy(new_op->upcall.req.mkdir.d_name,
- dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.mkdir.d_name,
+ dentry->d_name.name, ORANGEFS_NAME_MAX);
ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
@@ -405,12 +404,10 @@ static int orangefs_rename(struct mnt_idmap *idmap,
new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn;
new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn;
- strncpy(new_op->upcall.req.rename.d_old_name,
- old_dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
- strncpy(new_op->upcall.req.rename.d_new_name,
- new_dentry->d_name.name,
- ORANGEFS_NAME_MAX - 1);
+ strscpy(new_op->upcall.req.rename.d_old_name,
+ old_dentry->d_name.name, ORANGEFS_NAME_MAX);
+ strscpy(new_op->upcall.req.rename.d_new_name,
+ new_dentry->d_name.name, ORANGEFS_NAME_MAX);
ret = service_operation(new_op,
"orangefs_rename",
diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
index d990f4356b30..c714380ab38b 100644
--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -256,7 +256,7 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb)
new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT);
if (!new_op)
return -ENOMEM;
- strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
+ strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
orangefs_sb->devname,
ORANGEFS_MAX_SERVER_ADDR_LEN);
@@ -403,8 +403,8 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname)
return -ENOMEM;
op->upcall.req.fs_umount.id = id;
op->upcall.req.fs_umount.fs_id = fs_id;
- strncpy(op->upcall.req.fs_umount.orangefs_config_server,
- devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
+ strscpy(op->upcall.req.fs_umount.orangefs_config_server,
+ devname, ORANGEFS_MAX_SERVER_ADDR_LEN);
r = service_operation(op, "orangefs_fs_umount", 0);
/* Not much to do about an error here. */
if (r)
@@ -497,9 +497,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
if (!new_op)
return ERR_PTR(-ENOMEM);
- strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
- devname,
- ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
+ strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
+ devname, ORANGEFS_MAX_SERVER_ADDR_LEN);
gossip_debug(GOSSIP_SUPER_DEBUG,
"Attempting ORANGEFS Mount via host %s\n",
@@ -546,9 +545,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
* on successful mount, store the devname and data
* used
*/
- strncpy(ORANGEFS_SB(sb)->devname,
- devname,
- ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
+ strscpy(ORANGEFS_SB(sb)->devname, devname,
+ ORANGEFS_MAX_SERVER_ADDR_LEN);
/* mount_pending must be cleared */
ORANGEFS_SB(sb)->mount_pending = 0;
--
2.39.2
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);
| ^~~~~~~~~~~~~~~~~~~~
As far as I can tell, this is harmless here because the truncation is
intentional, but using strscpy_pad() will avoid the warning since gcc
does not (yet) know about it.
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..c9820122af56 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);
+ strscpy_pad(p, data_a, l);
p += l;
}
--
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);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This one is intentional, so rewrite the code in a way that avoids the
warning. Since there is already an extra strlen() and an overflow check,
the actual size to be copied is already known here.
Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/acpi/acpica/tbfind.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/acpica/tbfind.c b/drivers/acpi/acpica/tbfind.c
index 1c1b2e284bd9..472ce2a6624b 100644
--- a/drivers/acpi/acpica/tbfind.c
+++ b/drivers/acpi/acpica/tbfind.c
@@ -36,7 +36,7 @@ acpi_tb_find_table(char *signature,
{
acpi_status status = AE_OK;
struct acpi_table_header header;
- u32 i;
+ u32 len, i;
ACPI_FUNCTION_TRACE(tb_find_table);
@@ -46,19 +46,18 @@ acpi_tb_find_table(char *signature,
return_ACPI_STATUS(AE_BAD_SIGNATURE);
}
- /* Don't allow the OEM strings to be too long */
-
- if ((strlen(oem_id) > ACPI_OEM_ID_SIZE) ||
- (strlen(oem_table_id) > ACPI_OEM_TABLE_ID_SIZE)) {
- return_ACPI_STATUS(AE_AML_STRING_LIMIT);
- }
-
/* Normalize the input strings */
memset(&header, 0, sizeof(struct acpi_table_header));
ACPI_COPY_NAMESEG(header.signature, signature);
- strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
- strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
+ len = strlen(oem_id);
+ if (len > ACPI_OEM_ID_SIZE)
+ return_ACPI_STATUS(AE_AML_STRING_LIMIT);
+ memcpy(header.oem_id, oem_id, len);
+ len = strlen(oem_table_id);
+ if (len > ACPI_OEM_TABLE_ID_SIZE)
+ return_ACPI_STATUS(AE_AML_STRING_LIMIT);
+ memcpy(header.oem_table_id, oem_table_id, len);
/* Search for the table */
--
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]>
---
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..95a00160d465 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(buts->name, name, BLKTRACE_BDEV_SIZE);
/*
* some device names have larger paths - convert the slashes
--
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]>
---
scripts/Makefile.extrawarn | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index aa1c716c4812..5a25f133d0e9 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -102,7 +102,6 @@ KBUILD_CFLAGS += $(call cc-disable-warning, format-overflow)
ifdef CONFIG_CC_IS_GCC
KBUILD_CFLAGS += $(call cc-disable-warning, format-truncation)
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-10 warns about a strncpy() that does not enforce zero-termination:
In file included from include/linux/string.h:369,
from drivers/staging/greybus/fw-management.c:9:
In function 'strncpy',
inlined from 'fw_mgmt_backend_fw_update_operation' at drivers/staging/greybus/fw-management.c:306:2:
include/linux/fortify-string.h:108:30: error: '__builtin_strncpy' specified bound 10 equals destination size [-Werror=stringop-truncation]
108 | #define __underlying_strncpy __builtin_strncpy
| ^
include/linux/fortify-string.h:187:9: note: in expansion of macro '__underlying_strncpy'
187 | return __underlying_strncpy(p, q, size);
| ^~~~~~~~~~~~~~~~~~~~
For some reason, I cannot reproduce this with gcc-9 or gcc-11, so I'm not
sure what's going on. Changing it to strspy() avoids the warning. In this
case the existing check for zero-termination would fail but can be replaced
with an error check.
Signed-off-by: Arnd Bergmann <[email protected]>
---
This is from randconfig testing with random gcc versions, a .config to
reproduce is at https://pastebin.com/r13yezkU
---
drivers/staging/greybus/fw-management.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
index 3054f084d777..35bfdd5f32d2 100644
--- a/drivers/staging/greybus/fw-management.c
+++ b/drivers/staging/greybus/fw-management.c
@@ -303,13 +303,13 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
struct gb_fw_mgmt_backend_fw_update_request request;
int ret;
- strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
+ ret = strscpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
/*
* The firmware-tag should be NULL terminated, otherwise throw error and
* fail.
*/
- if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
+ if (ret == -E2BIG) {
dev_err(fw_mgmt->parent, "backend-update: firmware-tag is not NULL terminated\n");
return -EINVAL;
}
--
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.
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]>
gcc-9 complains about a possibly unterminated string in the strncpy() destination:
In function 'rtw_cfg80211_add_monitor_if',
inlined from 'cfg80211_rtw_add_virtual_intf' at drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2209:9:
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2146:2: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
2146 | strncpy(mon_ndev->name, name, IFNAMSIZ);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This one is a false-positive because of the explicit termination in the following
line, and recent versions of clang and gcc no longer warn about this.
Interestingly, the other strncpy() in this file is missing a termination but
does not produce a warning, possibly because of the type confusion and the
cast between u8 and char.
Change both strncpy() instances to strscpy(), which avoids the warning as well
as the possibly missing termination. No additional padding is needed here.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 65a450fcdce7..98bc5520e77d 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -884,7 +884,7 @@ static int cfg80211_rtw_add_key(struct wiphy *wiphy, struct net_device *ndev,
goto addkey_end;
}
- strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
+ strscpy(param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
if (!mac_addr || is_broadcast_ether_addr(mac_addr))
param->u.crypt.set_tx = 0; /* for wpa/wpa2 group key */
@@ -2143,8 +2143,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
}
mon_ndev->type = ARPHRD_IEEE80211_RADIOTAP;
- strncpy(mon_ndev->name, name, IFNAMSIZ);
- mon_ndev->name[IFNAMSIZ - 1] = 0;
+ strscpy(mon_ndev->name, name, IFNAMSIZ);
mon_ndev->needs_free_netdev = true;
mon_ndev->priv_destructor = rtw_ndev_destructor;
--
2.39.2
On Thu, 28 Mar 2024 15:04:52 +0100
Arnd Bergmann <[email protected]> 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]>
> ---
> 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..95a00160d465 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(buts->name, name, BLKTRACE_BDEV_SIZE);
The commit message says "Using strscpy_pad()" but it doesn't do so in the
patch.
Rule 12 of debugging: "When the comment and the code do not match, they are
probably both wrong"
-- Steve
>
> /*
> * some device names have larger paths - convert the slashes
On Thu, Mar 28, 2024 at 03:04:45PM +0100, Arnd Bergmann wrote:
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index 258aa0e37f55..6ca5797aeae5 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -937,8 +937,8 @@ static int create_component(struct vchiq_mmal_instance *instance,
> /* build component create message */
> m.h.type = MMAL_MSG_TYPE_COMPONENT_CREATE;
> m.u.component_create.client_component = component->client_component;
> - strncpy(m.u.component_create.name, name,
> - sizeof(m.u.component_create.name));
> + strscpy_pad(m.u.component_create.name, name,
> + sizeof(m.u.component_create.name));
You sent this earlier and we already applied it.
Btw, I just learned there is a new trick to write this when it's just
sizeof(dest).
strscpy_pad(m.u.component_create.name, name);
regards,
dan carpenter
On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> This is from randconfig testing with random gcc versions, a .config to
> reproduce is at https://pastebin.com/r13yezkU
> ---
> drivers/staging/greybus/fw-management.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index 3054f084d777..35bfdd5f32d2 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -303,13 +303,13 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
> struct gb_fw_mgmt_backend_fw_update_request request;
> int ret;
>
> - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> + ret = strscpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
This needs to be strscpy_pad() or it risks an information leak.
>
> /*
> * The firmware-tag should be NULL terminated, otherwise throw error and
^^^^^^^^^^^^^^^^
These comments are out of date.
> * fail.
> */
> - if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> + if (ret == -E2BIG) {
> dev_err(fw_mgmt->parent, "backend-update: firmware-tag is not NULL terminated\n");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
More out of date prints.
regards,
dan carpenter
On Thu, Mar 28, 2024, at 15:42, Dan Carpenter wrote:
> On Thu, Mar 28, 2024 at 03:04:45PM +0100, Arnd Bergmann wrote:
>> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>> index 258aa0e37f55..6ca5797aeae5 100644
>> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
>> @@ -937,8 +937,8 @@ static int create_component(struct vchiq_mmal_instance *instance,
>> /* build component create message */
>> m.h.type = MMAL_MSG_TYPE_COMPONENT_CREATE;
>> m.u.component_create.client_component = component->client_component;
>> - strncpy(m.u.component_create.name, name,
>> - sizeof(m.u.component_create.name));
>> + strscpy_pad(m.u.component_create.name, name,
>> + sizeof(m.u.component_create.name));
>
> You sent this earlier and we already applied it.
Sorry about that. I normally mark patches I send as applied
in the subject but it looks like I lost the annotation
by accident.
> Btw, I just learned there is a new trick to write this when it's just
> sizeof(dest).
>
> strscpy_pad(m.u.component_create.name, name);
Ah, cute.
arnd
On Thu, Mar 28, 2024 at 03:04:47PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> When -Wstringop-truncation is enabled, gcc finds a function that
> always does a short copy:
>
> In function 'inquiry',
> inlined from 'rtsx_scsi_handler' at drivers/staging/rts5208/rtsx_scsi.c:3210:12:
> drivers/staging/rts5208/rtsx_scsi.c:526:17: error: 'strncpy' output truncated copying between 1 and 28 bytes from a string of length 28 [-Werror=stringop-truncation]
> 526 | strncpy(buf + 8, inquiry_string, sendbytes - 8);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Since the actual size of the copy is already known at this point, just
> copy the bytes directly and skip the length check and zero-padding.
>
> This partially reverts an earlier bugfix that replaced the original
> incorrect memcpy() with a less bad strncpy(), but it now also avoids
> the original overflow.
>
> Fixes: 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
I don't see a problem with this commit. The "sendbytes - 8" prevents
a write overflow to buf, and the strncpy() prevents read overflow from
inquiry_string.
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/staging/rts5208/rtsx_scsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
> index 08bd768ad34d..a73b0959f5a9 100644
> --- a/drivers/staging/rts5208/rtsx_scsi.c
> +++ b/drivers/staging/rts5208/rtsx_scsi.c
> @@ -523,7 +523,7 @@ static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
>
> if (sendbytes > 8) {
> memcpy(buf, inquiry_buf, 8);
> - strncpy(buf + 8, inquiry_string, sendbytes - 8);
> + memcpy(buf + 8, inquiry_string, min(sendbytes, 36) - 8);
I think your math is off. The string is 29 characters + NUL. So it
should be "min(sendbytes, 38) - 8". You're chopping off the space and
the NUL terminator.
This only affects pro_formatter_flag code...
This code is such a mess. I'm not sure your fix is the complete fix.
When I see code that's clearly buggy like this and it's not sure the fix
is complete then I generally prefer to leave the static checker warning
as is so that we are reminded of the bug occasionally. How close are
you to removing all these -Wstringop-truncation warnings? Maybe we just
add a comment or a TODO item in the drivers/staging/rts5208/TODO file.
regards,
dan carpenter
On 3/28/24 07:04, Arnd Bergmann wrote:
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index ba7237e83863..58726c15ebac 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -290,18 +290,28 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key)
> static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
> char *from, int compatible)
> {
> - size_t from_length;
> + int ret;
>
> - from_length = strlen(from);
> - /* This zero-pads the destination */
> - strncpy(to, from, to_length);
> - if (from_length < to_length && !compatible) {
> - /*
> - * space pad the string if it is short.
> - */
> - memset(&to[from_length], ' ', to_length - from_length);
> + if (compatible) {
> + /* This zero-pads and nul-terminates the destination */
> + ret = strscpy_pad(to, from, to_length);
> + } else {
> + /* no nul-termination but space-padding for short strings */
> + size_t from_length = strlen(from);
> + ret = from_length;
> +
> + if (from_length > to_length) {
> + from_length = to_length;
> + ret = -E2BIG;
> + }
> +
> + memcpy(to, from, from_length);
> +
> + if (from_length < to_length)
> + memset(&to[from_length], ' ', to_length - from_length);
> }
> - if (from_length > to_length)
> +
> + if (ret < 0)
> printk(KERN_WARNING "%s: %s string '%s' is too long\n",
> __func__, name, from);
> }
Please eliminate the variable 'ret'. I think that will improve
readability of the new code.
Thanks,
Bart.
Hi,
On Thu, Mar 28, 2024 at 7:07 AM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> gcc-9 complains about a possibly unterminated string in the strncpy() destination:
>
> In function 'rtw_cfg80211_add_monitor_if',
> inlined from 'cfg80211_rtw_add_virtual_intf' at drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2209:9:
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2146:2: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
> 2146 | strncpy(mon_ndev->name, name, IFNAMSIZ);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This one is a false-positive because of the explicit termination in the following
> line, and recent versions of clang and gcc no longer warn about this.
>
> Interestingly, the other strncpy() in this file is missing a termination but
> does not produce a warning, possibly because of the type confusion and the
> cast between u8 and char.
>
> Change both strncpy() instances to strscpy(), which avoids the warning as well
> as the possibly missing termination. No additional padding is needed here.
Could you also clean up the strncpy present in
drivers/staging/rtl8723bs/os_dep/os_intfs.c so all these are cleaned
up at once?
Maybe we could use the new 2-argument version of strscpy() introduced
in Commit e6584c3964f2f ("string: Allow 2-argument strscpy()") for all
3 of these too.
It looks like:
strscpy(dest, src);
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> index 65a450fcdce7..98bc5520e77d 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> @@ -884,7 +884,7 @@ static int cfg80211_rtw_add_key(struct wiphy *wiphy, struct net_device *ndev,
> goto addkey_end;
> }
>
> - strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
> + strscpy(param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
>
> if (!mac_addr || is_broadcast_ether_addr(mac_addr))
> param->u.crypt.set_tx = 0; /* for wpa/wpa2 group key */
> @@ -2143,8 +2143,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
> }
>
> mon_ndev->type = ARPHRD_IEEE80211_RADIOTAP;
> - strncpy(mon_ndev->name, name, IFNAMSIZ);
> - mon_ndev->name[IFNAMSIZ - 1] = 0;
> + strscpy(mon_ndev->name, name, IFNAMSIZ);
> mon_ndev->needs_free_netdev = true;
> mon_ndev->priv_destructor = rtw_ndev_destructor;
>
> --
> 2.39.2
>
Thanks
Justin
Hi,
On Thu, Mar 28, 2024 at 03:04:45PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc-14 warns about this strncpy() that results in a non-terminated
> string for an overflow:
>
> In file included from include/linux/string.h:369,
> from drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:20:
> In function 'strncpy',
> inlined from 'create_component' at drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:940:2:
> include/linux/fortify-string.h:108:33: error: '__builtin_strncpy' specified bound 128 equals destination size [-Werror=stringop-truncation]
>
> Change it to strscpy_pad(), which produces a properly terminated and
> zero-padded string.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
If there is other review that warrants a change, we might as well switch
over to the new 2-argument version of strscpy{_pad}() introduced in
Commit e6584c3964f2f ("string: Allow 2-argument strscpy()"). No need to
change for only this reason, though.
Reviewed-by: Justin Stitt <[email protected]>
> ---
> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index 258aa0e37f55..6ca5797aeae5 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -937,8 +937,8 @@ static int create_component(struct vchiq_mmal_instance *instance,
> /* build component create message */
> m.h.type = MMAL_MSG_TYPE_COMPONENT_CREATE;
> m.u.component_create.client_component = component->client_component;
> - strncpy(m.u.component_create.name, name,
> - sizeof(m.u.component_create.name));
> + strscpy_pad(m.u.component_create.name, name,
> + sizeof(m.u.component_create.name));
>
> ret = send_synchronous_mmal_msg(instance, &m,
> sizeof(m.u.component_create),
> --
> 2.39.2
>
Thanks
Justin
Hi,
On Thu, Mar 28, 2024 at 03:04:46PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> scsi_strcpy_devinfo() appears to work as intended but its semantics are
> so confusing that gcc warns about it when -Wstringop-truncation is enabled:
>
> In function 'scsi_strcpy_devinfo',
> inlined from 'scsi_dev_info_list_add_keyed' at drivers/scsi/scsi_devinfo.c:370:2:
> drivers/scsi/scsi_devinfo.c:297:9: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
> 297 | strncpy(to, from, to_length);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reorganize the function to completely separate the nul-terminated from
> the space-padded/non-terminated case. The former is just strscpy_pad(),
> while the latter does not have a standard function.
>
I did the same in a patch sent earlier (few weeks ago):
https://lore.kernel.org/all/20240305-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v3-5-5b78a13ff984@google.com/
Maybe reviewers can chime in on which version is preferred and go from
there.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/scsi/scsi_devinfo.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index ba7237e83863..58726c15ebac 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -290,18 +290,28 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key)
> static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
> char *from, int compatible)
> {
> - size_t from_length;
> + int ret;
>
> - from_length = strlen(from);
> - /* This zero-pads the destination */
> - strncpy(to, from, to_length);
> - if (from_length < to_length && !compatible) {
> - /*
> - * space pad the string if it is short.
> - */
> - memset(&to[from_length], ' ', to_length - from_length);
> + if (compatible) {
> + /* This zero-pads and nul-terminates the destination */
> + ret = strscpy_pad(to, from, to_length);
> + } else {
> + /* no nul-termination but space-padding for short strings */
> + size_t from_length = strlen(from);
> + ret = from_length;
> +
> + if (from_length > to_length) {
> + from_length = to_length;
> + ret = -E2BIG;
> + }
> +
> + memcpy(to, from, from_length);
> +
> + if (from_length < to_length)
> + memset(&to[from_length], ' ', to_length - from_length);
> }
> - if (from_length > to_length)
> +
> + if (ret < 0)
> printk(KERN_WARNING "%s: %s string '%s' is too long\n",
> __func__, name, from);
> }
> --
> 2.39.2
>
Thanks
Justin
On Fri, Mar 29, 2024, at 00:14, Justin Stitt wrote:
>
> On Thu, Mar 28, 2024 at 03:04:46PM +0100, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> scsi_strcpy_devinfo() appears to work as intended but its semantics are
>> so confusing that gcc warns about it when -Wstringop-truncation is enabled:
>>
>> In function 'scsi_strcpy_devinfo',
>> inlined from 'scsi_dev_info_list_add_keyed' at drivers/scsi/scsi_devinfo.c:370:2:
>> drivers/scsi/scsi_devinfo.c:297:9: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation]
>> 297 | strncpy(to, from, to_length);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reorganize the function to completely separate the nul-terminated from
>> the space-padded/non-terminated case. The former is just strscpy_pad(),
>> while the latter does not have a standard function.
>>
>
> I did the same in a patch sent earlier (few weeks ago):
>
> https://lore.kernel.org/all/20240305-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v3-5-5b78a13ff984@google.com/
>
> Maybe reviewers can chime in on which version is preferred and go from
> there.
I'm in favor of your version, it looks nicer and addresses the comment
that Bart had on mine.
Arnd
Hi,
On Thu, Mar 28, 2024 at 03:04:48PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc warns about a truncated string copy with a 255 byte string getting
> copied to a buffer of the same length, losing the 0-termination:
>
> In function 'orangefs_unmount',
> inlined from 'orangefs_kill_sb' at arm-soc/fs/orangefs/super.c:619:6:
> fs/orangefs/super.c:406:9: error: 'strncpy' output may be truncated copying 255 bytes from a string of length 255 [-Werror=stringop-truncation]
> 406 | strncpy(op->upcall.req.fs_umount.orangefs_config_server,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 407 | devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> I see that most string copies in orangefs are for the upcalls and use
> a buffer that is one short to get the implied termination from the
> zero-filled buffer, but some other instances lack the '-1' part.
>
> Convert from strncpy() to strscpy() to avoids both the warning about
> the buffer size and the need for the explicit padding, since strscpy
> guarantees a zero-terminated buffer.
>
I think I got most of these with my patch sent earlier last week:
https://lore.kernel.org/all/20240322-strncpy-fs-orangefs-dcache-c-v1-1-15d12debbf38@google.com/
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> fs/orangefs/dcache.c | 4 ++--
> fs/orangefs/namei.c | 33 +++++++++++++++------------------
> fs/orangefs/super.c | 16 +++++++---------
> 3 files changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
> index 8bbe9486e3a6..96ed9900f7a9 100644
> --- a/fs/orangefs/dcache.c
> +++ b/fs/orangefs/dcache.c
> @@ -33,9 +33,9 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
>
> new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
> new_op->upcall.req.lookup.parent_refn = parent->refn;
> - strncpy(new_op->upcall.req.lookup.d_name,
> + strscpy(new_op->upcall.req.lookup.d_name,
> dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + ORANGEFS_NAME_MAX);
>
> gossip_debug(GOSSIP_DCACHE_DEBUG,
> "%s:%s:%d interrupt flag [%d]\n",
> diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
> index c9dfd5c6a097..5e46d3bdcb05 100644
> --- a/fs/orangefs/namei.c
> +++ b/fs/orangefs/namei.c
> @@ -41,8 +41,8 @@ static int orangefs_create(struct mnt_idmap *idmap,
> fill_default_sys_attrs(new_op->upcall.req.create.attributes,
> ORANGEFS_TYPE_METAFILE, mode);
>
> - strncpy(new_op->upcall.req.create.d_name,
> - dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.create.d_name,
> + dentry->d_name.name, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -137,8 +137,8 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
> &parent->refn.khandle);
> new_op->upcall.req.lookup.parent_refn = parent->refn;
>
> - strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
> + ORANGEFS_NAME_MAX);
>
> gossip_debug(GOSSIP_NAME_DEBUG,
> "%s: doing lookup on %s under %pU,%d\n",
> @@ -192,8 +192,8 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry)
> return -ENOMEM;
>
> new_op->upcall.req.remove.parent_refn = parent->refn;
> - strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
> + ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, "orangefs_unlink",
> get_interruptible_flag(inode));
> @@ -247,10 +247,9 @@ static int orangefs_symlink(struct mnt_idmap *idmap,
> ORANGEFS_TYPE_SYMLINK,
> mode);
>
> - strncpy(new_op->upcall.req.sym.entry_name,
> - dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.sym.entry_name,
> + dentry->d_name.name, ORANGEFS_NAME_MAX);
> + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -324,8 +323,8 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes,
> ORANGEFS_TYPE_DIRECTORY, mode);
>
> - strncpy(new_op->upcall.req.mkdir.d_name,
> - dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.mkdir.d_name,
> + dentry->d_name.name, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -405,12 +404,10 @@ static int orangefs_rename(struct mnt_idmap *idmap,
> new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn;
> new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn;
>
> - strncpy(new_op->upcall.req.rename.d_old_name,
> - old_dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> - strncpy(new_op->upcall.req.rename.d_new_name,
> - new_dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.rename.d_old_name,
> + old_dentry->d_name.name, ORANGEFS_NAME_MAX);
> + strscpy(new_op->upcall.req.rename.d_new_name,
> + new_dentry->d_name.name, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op,
> "orangefs_rename",
> diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
> index d990f4356b30..c714380ab38b 100644
> --- a/fs/orangefs/super.c
> +++ b/fs/orangefs/super.c
> @@ -256,7 +256,7 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb)
> new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT);
> if (!new_op)
> return -ENOMEM;
> - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> orangefs_sb->devname,
> ORANGEFS_MAX_SERVER_ADDR_LEN);
>
> @@ -403,8 +403,8 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname)
> return -ENOMEM;
> op->upcall.req.fs_umount.id = id;
> op->upcall.req.fs_umount.fs_id = fs_id;
> - strncpy(op->upcall.req.fs_umount.orangefs_config_server,
> - devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(op->upcall.req.fs_umount.orangefs_config_server,
> + devname, ORANGEFS_MAX_SERVER_ADDR_LEN);
> r = service_operation(op, "orangefs_fs_umount", 0);
> /* Not much to do about an error here. */
> if (r)
> @@ -497,9 +497,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
> if (!new_op)
> return ERR_PTR(-ENOMEM);
>
> - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> - devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> + devname, ORANGEFS_MAX_SERVER_ADDR_LEN);
>
> gossip_debug(GOSSIP_SUPER_DEBUG,
> "Attempting ORANGEFS Mount via host %s\n",
> @@ -546,9 +545,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
> * on successful mount, store the devname and data
> * used
> */
> - strncpy(ORANGEFS_SB(sb)->devname,
> - devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(ORANGEFS_SB(sb)->devname, devname,
> + ORANGEFS_MAX_SERVER_ADDR_LEN);
>
> /* mount_pending must be cleared */
> ORANGEFS_SB(sb)->mount_pending = 0;
> --
> 2.39.2
>
Thanks
Justin
Hi,
On Thu, Mar 28, 2024 at 03:04:50PM +0100, 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);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This one is intentional, so rewrite the code in a way that avoids the
> warning. Since there is already an extra strlen() and an overflow check,
> the actual size to be copied is already known here.
>
I also tried cleaning these up but Kees informed me that this subsystem
is maintained elsewhere:
https://lore.kernel.org/all/202308241612.DFE4119@keescook/
I am not sure if you can get changes through by the traditional means.
>
> Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/acpi/acpica/tbfind.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/tbfind.c b/drivers/acpi/acpica/tbfind.c
> index 1c1b2e284bd9..472ce2a6624b 100644
> --- a/drivers/acpi/acpica/tbfind.c
> +++ b/drivers/acpi/acpica/tbfind.c
> @@ -36,7 +36,7 @@ acpi_tb_find_table(char *signature,
> {
> acpi_status status = AE_OK;
> struct acpi_table_header header;
> - u32 i;
> + u32 len, i;
>
> ACPI_FUNCTION_TRACE(tb_find_table);
>
> @@ -46,19 +46,18 @@ acpi_tb_find_table(char *signature,
> return_ACPI_STATUS(AE_BAD_SIGNATURE);
> }
>
> - /* Don't allow the OEM strings to be too long */
> -
> - if ((strlen(oem_id) > ACPI_OEM_ID_SIZE) ||
> - (strlen(oem_table_id) > ACPI_OEM_TABLE_ID_SIZE)) {
> - return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> - }
> -
> /* Normalize the input strings */
>
> memset(&header, 0, sizeof(struct acpi_table_header));
> ACPI_COPY_NAMESEG(header.signature, signature);
> - strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
> - strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
> + len = strlen(oem_id);
> + if (len > ACPI_OEM_ID_SIZE)
> + return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> + memcpy(header.oem_id, oem_id, len);
> + len = strlen(oem_table_id);
> + if (len > ACPI_OEM_TABLE_ID_SIZE)
> + return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> + memcpy(header.oem_table_id, oem_table_id, len);
>
> /* Search for the table */
>
> --
> 2.39.2
>
Thanks
Justin
Hi,
On Thu, Mar 28, 2024 at 03:04:51PM +0100, Arnd Bergmann wrote:
> 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.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Thanks for the patch!
This helps towards: https://github.com/KSPP/linux/issues/90
Reviewed-by: Justin Stitt <[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
>
Thanks
Justin
Hi,
On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> gcc-10 warns about a strncpy() that does not enforce zero-termination:
>
> In file included from include/linux/string.h:369,
> from drivers/staging/greybus/fw-management.c:9:
> In function 'strncpy',
> inlined from 'fw_mgmt_backend_fw_update_operation' at drivers/staging/greybus/fw-management.c:306:2:
> include/linux/fortify-string.h:108:30: error: '__builtin_strncpy' specified bound 10 equals destination size [-Werror=stringop-truncation]
> 108 | #define __underlying_strncpy __builtin_strncpy
> | ^
> include/linux/fortify-string.h:187:9: note: in expansion of macro '__underlying_strncpy'
> 187 | return __underlying_strncpy(p, q, size);
> | ^~~~~~~~~~~~~~~~~~~~
>
> For some reason, I cannot reproduce this with gcc-9 or gcc-11, so I'm not
> sure what's going on. Changing it to strspy() avoids the warning. In this
> case the existing check for zero-termination would fail but can be replaced
> with an error check.
>
>
Arnd, I see 4 instances of strncpy() in this file. Could you clean them
all up at once which would help GREATLY towards:
https://github.com/KSPP/linux/issues/90
strncpy() is an often misued and misunderstood (and deprecated [1])
string API, let's get rid of that thing all together!
[1]: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> This is from randconfig testing with random gcc versions, a .config to
> reproduce is at https://pastebin.com/r13yezkU
> ---
> drivers/staging/greybus/fw-management.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index 3054f084d777..35bfdd5f32d2 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -303,13 +303,13 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
> struct gb_fw_mgmt_backend_fw_update_request request;
> int ret;
>
> - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> + ret = strscpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
>
> /*
> * The firmware-tag should be NULL terminated, otherwise throw error and
> * fail.
> */
> - if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> + if (ret == -E2BIG) {
> dev_err(fw_mgmt->parent, "backend-update: firmware-tag is not NULL terminated\n");
> return -EINVAL;
> }
> --
> 2.39.2
>
Thanks
Justin
Hi,
On Thu, Mar 28, 2024 at 03:04:49PM +0100, 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);
> | ^~~~~~~~~~~~~~~~~~~~
>
> As far as I can tell, this is harmless here because the truncation is
> intentional, but using strscpy_pad() will avoid the warning since gcc
> does not (yet) know about it.
>
We need to be careful. strscpy() or strscpy_pad() are not drop-in
replacements for strncpy().
if @l is less than the length of @data_a we might have a problem because
strscpy_pad() will eagerly assign a NUL-byte to dest[l-1].
It looks like @l could be less than the length of @data_a judging from:
size_t len = get_random_u32_inclusive(1, d);
Let me model the potential behavior before and after, understanding that
data_a is defined as:
static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
Before (using strncpy):
p = ['j', 'u', 'n', 'k'] // example destination buffer
assume @l = 3
then we are trying to copy ".2." from @data_a, resulting in
p = ['.', '2', '.', 'k']
After (using strscpy_pad()):
p = ['j', 'u', 'n', 'k'] // example destination buffer
assume @l = 3
then we are trying to copy ".2." from @data_a, resulting in
p = ['.', '2', '\0', 'k']
because strscpy got to the end of its allowed size and didn't find a
NUL-term from its source string, so it eagerly assigns a NUL-byte to the
end, essentially truncating our string.
Here's the responsible code from strscpy's implementation:
if (res)
dest[res-1] = '\0';
https://elixir.bootlin.com/linux/latest/source/lib/string.c#L107
It is possible I haven't fully considered the context of this change but
I think using strscpy_pad() will cause these tests to fail, if they
aren't failing I think we're getting lucky.
Also, maybe this godbolt example can help demonstrate:
https://godbolt.org/z/nWGKraTvT
>
> 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..c9820122af56 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);
> + strscpy_pad(p, data_a, l);
> p += l;
> }
>
> --
> 2.39.2
>
Thanks
Justin
On Thu, Mar 28, 2024, at 17:35, Dan Carpenter wrote:
> On Thu, Mar 28, 2024 at 03:04:47PM +0100, Arnd Bergmann wrote:
>> This partially reverts an earlier bugfix that replaced the original
>> incorrect memcpy() with a less bad strncpy(), but it now also avoids
>> the original overflow.
>>
>> Fixes: 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
>
> I don't see a problem with this commit. The "sendbytes - 8" prevents
> a write overflow to buf, and the strncpy() prevents read overflow from
> inquiry_string.
I agree the commit did not introduce a runtime bug, but it did
introduce a warning about the string being truncated.
>> diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
>> index 08bd768ad34d..a73b0959f5a9 100644
>> --- a/drivers/staging/rts5208/rtsx_scsi.c
>> +++ b/drivers/staging/rts5208/rtsx_scsi.c
>> @@ -523,7 +523,7 @@ static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
>>
>> if (sendbytes > 8) {
>> memcpy(buf, inquiry_buf, 8);
>> - strncpy(buf + 8, inquiry_string, sendbytes - 8);
>> + memcpy(buf + 8, inquiry_string, min(sendbytes, 36) - 8);
>
> I think your math is off. The string is 29 characters + NUL. So it
> should be "min(sendbytes, 38) - 8". You're chopping off the space and
> the NUL terminator.
>
> This only affects pro_formatter_flag code...
The extra two bytes were clearly a mistake in the original version
at the time it got added to drivers/staging. Note how the code
immediately below it would overwrite the space and NUL byte again:
if (pro_formatter_flag) {
if (sendbytes > 36)
memcpy(buf + 36, formatter_inquiry_str, sendbytes - 36);
}
> This code is such a mess. I'm not sure your fix is the complete fix.
> When I see code that's clearly buggy like this and it's not sure the fix
> is complete then I generally prefer to leave the static checker warning
> as is so that we are reminded of the bug occasionally.
I still think my patch is correct here, but I'll remove the confusing
spaces at the end of the strings and try to improve the commit
text.
A more readable version of the code might construct the entire
56 byte buffer on the stack and then do a single memcpy, but I
think the simpler change is sufficient here.
> How close are
> you to removing all these -Wstringop-truncation warnings? Maybe we just
> add a comment or a TODO item in the drivers/staging/rts5208/TODO file.
I'm down to eight warnings for clang now (on x86, arm and arm64 randconfigs
as well as allmodconfig and defconfig elsewhere), and hope to have it
enabled by default in either 6.10 or 6.11 after those are all
addressed.
I think gcc shows more warnings because it analyses buffer sizes
across inlining, while clang only does it within a given function.
Arnd
On Thu, Mar 28, 2024 at 3:06 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);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This one is intentional, so rewrite the code in a way that avoids the
> warning. Since there is already an extra strlen() and an overflow check,
> the actual size to be copied is already known here.
>
> Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
> Signed-off-by: Arnd Bergmann <[email protected]>
Because ACPICA is an external project supplying code to the Linux
kernel, the way to change the ACPICA code in the kernel is to submit a
pull request to the upstream ACPICA project on GitHub and once that PR
has been merged, submit a Linux patch corresponding to it including
the Link: tag pointing to the PR in question and the git ID of the
corresponding upstream ACPICA commit.
However, note that upstream ACPICA changes are applied to the Linux
kernel source code every time the upstream ACPICA project makes a
release, so it is not necessary to send the corresponding Linux
patches for them unless in the cases when timing matters.
> ---
> drivers/acpi/acpica/tbfind.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/tbfind.c b/drivers/acpi/acpica/tbfind.c
> index 1c1b2e284bd9..472ce2a6624b 100644
> --- a/drivers/acpi/acpica/tbfind.c
> +++ b/drivers/acpi/acpica/tbfind.c
> @@ -36,7 +36,7 @@ acpi_tb_find_table(char *signature,
> {
> acpi_status status = AE_OK;
> struct acpi_table_header header;
> - u32 i;
> + u32 len, i;
>
> ACPI_FUNCTION_TRACE(tb_find_table);
>
> @@ -46,19 +46,18 @@ acpi_tb_find_table(char *signature,
> return_ACPI_STATUS(AE_BAD_SIGNATURE);
> }
>
> - /* Don't allow the OEM strings to be too long */
> -
> - if ((strlen(oem_id) > ACPI_OEM_ID_SIZE) ||
> - (strlen(oem_table_id) > ACPI_OEM_TABLE_ID_SIZE)) {
> - return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> - }
> -
> /* Normalize the input strings */
>
> memset(&header, 0, sizeof(struct acpi_table_header));
> ACPI_COPY_NAMESEG(header.signature, signature);
> - strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
> - strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
> + len = strlen(oem_id);
> + if (len > ACPI_OEM_ID_SIZE)
> + return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> + memcpy(header.oem_id, oem_id, len);
> + len = strlen(oem_table_id);
> + if (len > ACPI_OEM_TABLE_ID_SIZE)
> + return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> + memcpy(header.oem_table_id, oem_table_id, len);
>
> /* Search for the table */
>
> --
> 2.39.2
>
>
On Fri, Mar 29, 2024, at 00:54, Justin Stitt wrote:
> On Thu, Mar 28, 2024 at 03:04:49PM +0100, 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);
>> | ^~~~~~~~~~~~~~~~~~~~
>>
>> As far as I can tell, this is harmless here because the truncation is
>> intentional, but using strscpy_pad() will avoid the warning since gcc
>> does not (yet) know about it.
>>
>
> We need to be careful. strscpy() or strscpy_pad() are not drop-in
> replacements for strncpy().
[cut useful explanation]
> It is possible I haven't fully considered the context of this change but
> I think using strscpy_pad() will cause these tests to fail, if they
> aren't failing I think we're getting lucky.
You are correct. I do understand the nuances between strncpy()
and strscpy(), but I failed to read this file properly.
I'm still not entirely sure, but from my current reading, I think
we can just use memcpy() to replace the strncpy() here, as both
the input string data_b[] and the output real[TEST_HEXDUMP_BUF_SIZE]
are sized to cover every possible 'len' value. This also follows
what Linus did for the other original strncpy in b1286ed7158e
("test_hexdump: use memcpy instead of strncpy()").
I've reworked the patch based on that assumption now and rewritten
the changelog text accordingly.
Arnd
On Mon, Apr 08, 2024 at 04:45:55PM +0200, Arnd Bergmann wrote:
> On Thu, Mar 28, 2024, at 17:35, Dan Carpenter wrote:
> > On Thu, Mar 28, 2024 at 03:04:47PM +0100, Arnd Bergmann wrote:
> >> This partially reverts an earlier bugfix that replaced the original
> >> incorrect memcpy() with a less bad strncpy(), but it now also avoids
> >> the original overflow.
> >>
> >> Fixes: 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
> >
> > I don't see a problem with this commit. The "sendbytes - 8" prevents
> > a write overflow to buf, and the strncpy() prevents read overflow from
> > inquiry_string.
>
> I agree the commit did not introduce a runtime bug, but it did
> introduce a warning about the string being truncated.
>
> >> diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
> >> index 08bd768ad34d..a73b0959f5a9 100644
> >> --- a/drivers/staging/rts5208/rtsx_scsi.c
> >> +++ b/drivers/staging/rts5208/rtsx_scsi.c
> >> @@ -523,7 +523,7 @@ static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
> >>
> >> if (sendbytes > 8) {
> >> memcpy(buf, inquiry_buf, 8);
> >> - strncpy(buf + 8, inquiry_string, sendbytes - 8);
> >> + memcpy(buf + 8, inquiry_string, min(sendbytes, 36) - 8);
> >
> > I think your math is off. The string is 29 characters + NUL. So it
> > should be "min(sendbytes, 38) - 8". You're chopping off the space and
> > the NUL terminator.
> >
> > This only affects pro_formatter_flag code...
>
> The extra two bytes were clearly a mistake in the original version
> at the time it got added to drivers/staging. Note how the code
> immediately below it would overwrite the space and NUL byte again:
>
> if (pro_formatter_flag) {
> if (sendbytes > 36)
> memcpy(buf + 36, formatter_inquiry_str, sendbytes - 36);
> }
>
Ah. Okay. Fair enough.
I do think this code is really suspect... What is the point of allowing
sendbytes < 36? But that's not related to your patch.
regards,
dan carpenter
On Thu, Mar 28, 2024, at 15:14, Steven Rostedt wrote:
> On Thu, 28 Mar 2024 15:04:52 +0100
>>
>> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
>> index d5d94510afd3..95a00160d465 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(buts->name, name, BLKTRACE_BDEV_SIZE);
>
> The commit message says "Using strscpy_pad()" but it doesn't do so in the
> patch.
>
> Rule 12 of debugging: "When the comment and the code do not match, they are
> probably both wrong"
Thanks for double-checking this, I had a hard time deciding which
one to use here and ended up with an obviously inconsistent version.
I've changed it now to strscpy_pad() for v2, which is the slightly
safer choice here. The non-padding version would still not leak
kernel data but would write back user-provided data after the
padding instead of always zeroing it.
Arnd
On Thu, Mar 28, 2024, at 16:00, Dan Carpenter wrote:
> On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> ---
>> This is from randconfig testing with random gcc versions, a .config to
>> reproduce is at https://pastebin.com/r13yezkU
>> ---
>> drivers/staging/greybus/fw-management.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
>> index 3054f084d777..35bfdd5f32d2 100644
>> --- a/drivers/staging/greybus/fw-management.c
>> +++ b/drivers/staging/greybus/fw-management.c
>> @@ -303,13 +303,13 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
>> struct gb_fw_mgmt_backend_fw_update_request request;
>> int ret;
>>
>> - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
>> + ret = strscpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
>
> This needs to be strscpy_pad() or it risks an information leak.
Right, I think I misread the code thinking that the strncpy()
destination was user provided, but I see now that this copy is
from user-provided data into the stack, so the padding is indeed
stale stack data.
I could not find out whether this gets copied back to userspace,
but adding the padding is safer indeed.
>>
>> /*
>> * The firmware-tag should be NULL terminated, otherwise throw error and
> ^^^^^^^^^^^^^^^^
> These comments are out of date.
>
>> * fail.
>> */
>> - if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
>> + if (ret == -E2BIG) {
>> dev_err(fw_mgmt->parent, "backend-update: firmware-tag is not NULL terminated\n");
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> More out of date prints.
I had thought about changing it when I did the patch, but could
not come up with anything that describes the error condition better:
the cause of the -E2BIG error is still the missing NUL-termination
in the provided string.
Maybe we should instead not print a warning at all? The general
rule is that user triggered operations should not lead to
spamming the kernel logs.
Arnd
On Fri, Mar 29, 2024, at 00:28, Justin Stitt wrote:
> On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
>>
>>
> Arnd, I see 4 instances of strncpy() in this file. Could you clean them
> all up at once which would help GREATLY towards:
> https://github.com/KSPP/linux/issues/90
Right, I see they all operate on the same string, so it makes
sense to keep these changes together. As Dan suggested, I'm using
the padding variant for all of these here, even though I'm not
entirely sure if this is required.
Arnd
On Fri, Mar 29, 2024, at 00:01, Justin Stitt wrote:
>> Change both strncpy() instances to strscpy(), which avoids the warning as well
>> as the possibly missing termination. No additional padding is needed here.
>
> Could you also clean up the strncpy present in
> drivers/staging/rtl8723bs/os_dep/os_intfs.c so all these are cleaned
> up at once?
I originally tried not to mix the general conversion with the
warning fixes, but it looks like it has the same issue in theory.
Not sure why there is no warning for this one, I guess it's because
it copies from a fixed-size source of the same length?
Anyway, it's clearly related here so I've added this for v2.
> Maybe we could use the new 2-argument version of strscpy() introduced
> in Commit e6584c3964f2f ("string: Allow 2-argument strscpy()") for all
> 3 of these too.
>
> It looks like:
> strscpy(dest, src);
>
Done now, after double-checking that the sizes actually match.
Thanks for the review,
Arnd
Hi,
On Mon, Apr 8, 2024 at 8:38 AM Arnd Bergmann <[email protected]> wrote:
>
> You are correct. I do understand the nuances between strncpy()
> and strscpy(), but I failed to read this file properly.
Arnd, I know you understand these differences. I did not intend to
patronize, so sorry about that. My intention was to provide ample
context for future travelers/reviewers. These replacements can be
tricky sometimes.
>
> I'm still not entirely sure, but from my current reading, I think
> we can just use memcpy() to replace the strncpy() here, as both
> the input string data_b[] and the output real[TEST_HEXDUMP_BUF_SIZE]
> are sized to cover every possible 'len' value. This also follows
> what Linus did for the other original strncpy in b1286ed7158e
> ("test_hexdump: use memcpy instead of strncpy()").
>
> I've reworked the patch based on that assumption now and rewritten
> the changelog text accordingly.
Great! This helps towards https://github.com/KSPP/linux/issues/90
>
> Arnd
Thanks
Justin
On Mon, Apr 8, 2024, at 17:59, Dan Carpenter wrote:
> On Mon, Apr 08, 2024 at 04:45:55PM +0200, Arnd Bergmann wrote:
>> On Thu, Mar 28, 2024, at 17:35, Dan Carpenter wrote:
>> >> if (sendbytes > 8) {
>> >> memcpy(buf, inquiry_buf, 8);
>> >> - strncpy(buf + 8, inquiry_string, sendbytes - 8);
>> >> + memcpy(buf + 8, inquiry_string, min(sendbytes, 36) - 8);
>> >
>> > I think your math is off. The string is 29 characters + NUL. So it
>> > should be "min(sendbytes, 38) - 8". You're chopping off the space and
>> > the NUL terminator.
>> >
>> > This only affects pro_formatter_flag code...
>>
>> The extra two bytes were clearly a mistake in the original version
>> at the time it got added to drivers/staging. Note how the code
>> immediately below it would overwrite the space and NUL byte again:
>>
>> if (pro_formatter_flag) {
>> if (sendbytes > 36)
>> memcpy(buf + 36, formatter_inquiry_str, sendbytes - 36);
>> }
>>
>
> Ah. Okay. Fair enough.
>
> I do think this code is really suspect... What is the point of allowing
> sendbytes < 36? But that's not related to your patch.
As far as I can tell, the driver tries to emulate the behavior
or normal scsi commands that could be issued from userspace through
SGIO with a short length. drivers/ata/libata-scsi.c has code to
handle INQUIRY as well that is somewhat similar but also different
enough that I don't trust the rts5208 version of it.
On a separate note, I just noticed that I forgot to put
the driver name into the subject line, which I've fixed
up for v2 as well now.
Arnd
On Mon, Apr 08, 2024 at 08:26:00PM +0200, Arnd Bergmann wrote:
> On Thu, Mar 28, 2024, at 16:00, Dan Carpenter wrote:
> > On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
> >> Signed-off-by: Arnd Bergmann <[email protected]>
> >> ---
> >> This is from randconfig testing with random gcc versions, a .config to
> >> reproduce is at https://pastebin.com/r13yezkU
> >> ---
> >> drivers/staging/greybus/fw-management.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> >> index 3054f084d777..35bfdd5f32d2 100644
> >> --- a/drivers/staging/greybus/fw-management.c
> >> +++ b/drivers/staging/greybus/fw-management.c
> >> @@ -303,13 +303,13 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
> >> struct gb_fw_mgmt_backend_fw_update_request request;
> >> int ret;
> >>
> >> - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> >> + ret = strscpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> >
> > This needs to be strscpy_pad() or it risks an information leak.
>
> Right, I think I misread the code thinking that the strncpy()
> destination was user provided, but I see now that this copy is
> from user-provided data into the stack, so the padding is indeed
> stale stack data.
>
> I could not find out whether this gets copied back to userspace,
> but adding the padding is safer indeed.
>
Grey bus is a bus, I'm not sure what's on the other end of the bus but
I think we've generally said that the data needs to be zeroed...
Although if that is true, why didn't I make this a Smatch warning?
regards,
dan carpenter