[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 01/11] staging: vc04_services: changen strncpy() to strscpy_pad()

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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

[PATCH 03/11] staging: replace weird strncpy() with memcpy()

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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

[PATCH 09/11] staging: rtl8723bs: convert strncpy to strscpy

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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

[PATCH 10/11] staging: greybus: change strncpy() to strscpy()

[Arnd Bergmann]

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;
 	}
-- 
2.39.2

Re: [PATCH 01/11] staging: vc04_services: changen strncpy() to strscpy_pad()

[Dan Carpenter]

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

Re: [PATCH 10/11] staging: greybus: change strncpy() to strscpy()

[Dan Carpenter]

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

Re: [PATCH 01/11] staging: vc04_services: changen strncpy() to strscpy_pad()

[Arnd Bergmann]

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

Re: [PATCH 03/11] staging: replace weird strncpy() with memcpy()

[Dan Carpenter]

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

Re: [PATCH 09/11] staging: rtl8723bs: convert strncpy to strscpy

[Justin Stitt]

Hi,

On Thu, Mar 28, 2024 at 7:07 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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 <arnd@arndb.de>
> ---
>  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

Re: [PATCH 01/11] staging: vc04_services: changen strncpy() to strscpy_pad()

[Justin Stitt]

Hi,

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

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 <justinstitt@google.com>

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

Re: [PATCH 10/11] staging: greybus: change strncpy() to strscpy()

[Justin Stitt]

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

Re: [PATCH 03/11] staging: replace weird strncpy() with memcpy()

[Arnd Bergmann]

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

Re: [PATCH 03/11] staging: replace weird strncpy() with memcpy()

[Dan Carpenter]

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

Re: [PATCH 09/11] staging: rtl8723bs: convert strncpy to strscpy

[Arnd Bergmann]

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

Re: [PATCH 10/11] staging: greybus: change strncpy() to strscpy()

[Arnd Bergmann]

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

Re: [PATCH 10/11] staging: greybus: change strncpy() to strscpy()

[Arnd Bergmann]

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

Re: [PATCH 03/11] staging: replace weird strncpy() with memcpy()

[Arnd Bergmann]

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

Re: [PATCH 10/11] staging: greybus: change strncpy() to strscpy()

[Dan Carpenter]

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