From: Arnd Bergmann arnd@arndb.de
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(-)
From: Arnd Bergmann arnd@arndb.de
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 arnd@arndb.de --- 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; }
On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
Signed-off-by: Arnd Bergmann arnd@arndb.de
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
Hi,
On Thu, Mar 28, 2024 at 03:04:54PM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
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 arnd@arndb.de
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
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 arnd@arndb.de
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 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 arnd@arndb.de
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
-
Arnd Bergmann -
Arnd Bergmann -
Dan Carpenter -
Justin Stitt