[PATCH 00/11] address remaining stringop-truncation warnings

[PATCH 00/11] address remaining stringop-truncation warnings

[Arnd Bergmann]

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(-)

-- 
2.39.2

Cc: "Richard Russon
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Robert Moore <robert.moore@intel.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Viresh Kumar <vireshk@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Martin Brandenburg <martin@omnibond.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nicolas Schier <nicolas@fjasle.eu>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: linux-ntfs-dev@lists.sourceforge.net
Cc: linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Cc: acpica-devel@lists.linux.dev
Cc: linux-scsi@vger.kernel.org
Cc: greybus-dev@lists.linaro.org
Cc: linux-staging@lists.linux.dev
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: devel@lists.orangefs.org
Cc: linux-trace-kernel@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org

[PATCH 07/11] block/partitions/ldm: convert strncpy() to strscpy()

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 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

[PATCH 08/11] blktrace: convert strncpy() to strscpy_pad()

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 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

Re: [PATCH 08/11] blktrace: convert strncpy() to strscpy_pad()

[Steven Rostedt]

On Thu, 28 Mar 2024 15:04:52 +0100
Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>
> ---
>  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

Re: [PATCH 07/11] block/partitions/ldm: convert strncpy() to strscpy()

[Justin Stitt]

Hi,

On Thu, Mar 28, 2024 at 03:04:51PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>

Thanks for the patch!

This helps towards: https://github.com/KSPP/linux/issues/90

Reviewed-by: Justin Stitt <justinstitt@google.com>

> ---
>  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

Re: [PATCH 08/11] blktrace: convert strncpy() to strscpy_pad()

[Arnd Bergmann]

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