[PATCH 1/3] [v2] staging: rts5208: replace weird strncpy() with memcpy()

[PATCH 1/3] [v2] staging: rts5208: 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);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The code originally had a memcpy() that would overread the source string,
and commit 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
fixed this but introduced the warning about truncation in the process.

As Dan points out, the final space in the inquiry_string always gets
cut off, so remove it here for clarity, leaving exactly the 28 non-NUL
characters that can get copied into the output. In the 'pro_formatter_flag'
this is followed by another 20 bytes from the 'formatter_inquiry_str'
array, but there the output never contains a NUL-termination, and the
length is known, so memcpy() is the more logical choice.

Cc: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/lkml/695be581-548f-4e5e-a211-5f3b95568e77@moroto.mountain/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: remove unneeded space byte from input string for clarity,
    rework changelog text
---
 drivers/staging/rts5208/rtsx_scsi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
index 08bd768ad34d..c27cffb9ad8f 100644
--- a/drivers/staging/rts5208/rtsx_scsi.c
+++ b/drivers/staging/rts5208/rtsx_scsi.c
@@ -463,10 +463,10 @@ static unsigned char formatter_inquiry_str[20] = {
 static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 {
 	unsigned int lun = SCSI_LUN(srb);
-	char *inquiry_default = (char *)"Generic-xD/SD/M.S.      1.00 ";
-	char *inquiry_sdms =    (char *)"Generic-SD/MemoryStick  1.00 ";
-	char *inquiry_sd =      (char *)"Generic-SD/MMC          1.00 ";
-	char *inquiry_ms =      (char *)"Generic-MemoryStick     1.00 ";
+	char *inquiry_default = (char *)"Generic-xD/SD/M.S.      1.00";
+	char *inquiry_sdms =    (char *)"Generic-SD/MemoryStick  1.00";
+	char *inquiry_sd =      (char *)"Generic-SD/MMC          1.00";
+	char *inquiry_ms =      (char *)"Generic-MemoryStick     1.00";
 	char *inquiry_string;
 	unsigned char sendbytes;
 	unsigned char *buf;
@@ -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 2/3] [v2] 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>
---
v2:
  use the two-argument version of strscpy(), which is simpler for the constant
  size destination. Add the third instance in this driver as well.
---
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 5 ++---
 drivers/staging/rtl8723bs/os_dep/os_intfs.c       | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 65a450fcdce7..3fe27ee75b47 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);
 
 	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);
 	mon_ndev->needs_free_netdev = true;
 	mon_ndev->priv_destructor = rtw_ndev_destructor;
 
diff --git a/drivers/staging/rtl8723bs/os_dep/os_intfs.c b/drivers/staging/rtl8723bs/os_dep/os_intfs.c
index 68bba3c0e757..55d0140cd543 100644
--- a/drivers/staging/rtl8723bs/os_dep/os_intfs.c
+++ b/drivers/staging/rtl8723bs/os_dep/os_intfs.c
@@ -415,7 +415,7 @@ static int rtw_ndev_init(struct net_device *dev)
 	struct adapter *adapter = rtw_netdev_priv(dev);
 
 	netdev_dbg(dev, FUNC_ADPT_FMT "\n", FUNC_ADPT_ARG(adapter));
-	strncpy(adapter->old_ifname, dev->name, IFNAMSIZ);
+	strscpy(adapter->old_ifname, dev->name);
 
 	return 0;
 }
-- 
2.39.2

Re: [PATCH 2/3] [v2] staging: rtl8723bs: convert strncpy to strscpy

[Justin Stitt]

Hi,

On Mon, Apr 08, 2024 at 09:48:10PM +0200, Arnd Bergmann 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.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Helps with: https://github.com/KSPP/linux/issues/90

and for the bots...
Link: https://github.com/KSPP/linux/issues/90

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

> ---
> v2:
>   use the two-argument version of strscpy(), which is simpler for the constant
>   size destination. Add the third instance in this driver as well.
> ---
>  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 5 ++---
>  drivers/staging/rtl8723bs/os_dep/os_intfs.c       | 2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> index 65a450fcdce7..3fe27ee75b47 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);
>  
>  	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);
>  	mon_ndev->needs_free_netdev = true;
>  	mon_ndev->priv_destructor = rtw_ndev_destructor;
>  
> diff --git a/drivers/staging/rtl8723bs/os_dep/os_intfs.c b/drivers/staging/rtl8723bs/os_dep/os_intfs.c
> index 68bba3c0e757..55d0140cd543 100644
> --- a/drivers/staging/rtl8723bs/os_dep/os_intfs.c
> +++ b/drivers/staging/rtl8723bs/os_dep/os_intfs.c
> @@ -415,7 +415,7 @@ static int rtw_ndev_init(struct net_device *dev)
>  	struct adapter *adapter = rtw_netdev_priv(dev);
>  
>  	netdev_dbg(dev, FUNC_ADPT_FMT "\n", FUNC_ADPT_ARG(adapter));
> -	strncpy(adapter->old_ifname, dev->name, IFNAMSIZ);
> +	strscpy(adapter->old_ifname, dev->name);
>  
>  	return 0;
>  }
> -- 
> 2.39.2
> 
>

Thanks
Justin

Re: [PATCH 2/3] [v2] staging: rtl8723bs: convert strncpy to strscpy

[Dan Carpenter]

On Mon, Apr 08, 2024 at 09:48:10PM +0200, Arnd Bergmann 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.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2:
>   use the two-argument version of strscpy(), which is simpler for the constant
>   size destination. Add the third instance in this driver as well.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

[PATCH 3/3] [v2] staging: greybus: change strncpy() to strscpy_pad()

[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, and I only
get a warning for one of the four related strncpy()s, so I'm not
sure what's going on.

Change all four to strscpy_pad(), which is the safest replacement here,
as it avoids ending up with uninitialized stack data in the tag name.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2
 - use strscpy_pad()
 - use two-argument form
 - change all four instances, not just the one that produced the warning
---
 drivers/staging/greybus/fw-management.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
index 3054f084d777..a47385175582 100644
--- a/drivers/staging/greybus/fw-management.c
+++ b/drivers/staging/greybus/fw-management.c
@@ -123,8 +123,7 @@ static int fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt,
 	fw_info->major = le16_to_cpu(response.major);
 	fw_info->minor = le16_to_cpu(response.minor);
 
-	strncpy(fw_info->firmware_tag, response.firmware_tag,
-		GB_FIRMWARE_TAG_MAX_SIZE);
+	strscpy_pad(fw_info->firmware_tag, response.firmware_tag);
 
 	/*
 	 * The firmware-tag should be NULL terminated, otherwise throw error but
@@ -153,7 +152,7 @@ static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
 	}
 
 	request.load_method = load_method;
-	strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
+	strscpy_pad(request.firmware_tag, tag);
 
 	/*
 	 * The firmware-tag should be NULL terminated, otherwise throw error and
@@ -249,8 +248,7 @@ static int fw_mgmt_backend_fw_version_operation(struct fw_mgmt *fw_mgmt,
 	struct gb_fw_mgmt_backend_fw_version_response response;
 	int ret;
 
-	strncpy(request.firmware_tag, fw_info->firmware_tag,
-		GB_FIRMWARE_TAG_MAX_SIZE);
+	strscpy_pad(request.firmware_tag, fw_info->firmware_tag);
 
 	/*
 	 * The firmware-tag should be NULL terminated, otherwise throw error and
@@ -303,13 +301,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_pad(request.firmware_tag, tag);
 
 	/*
 	 * 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 3/3] [v2] staging: greybus: change strncpy() to strscpy_pad()

[Justin Stitt]

Hi,

On Mon, Apr 08, 2024 at 09:48:11PM +0200, 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, and I only
> get a warning for one of the four related strncpy()s, so I'm not
> sure what's going on.
> 
> Change all four to strscpy_pad(), which is the safest replacement here,
> as it avoids ending up with uninitialized stack data in the tag name.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

This patch helps out with [1].

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

> ---
> v2
>  - use strscpy_pad()
>  - use two-argument form
>  - change all four instances, not just the one that produced the warning
> ---
>  drivers/staging/greybus/fw-management.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index 3054f084d777..a47385175582 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -123,8 +123,7 @@ static int fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt,
>  	fw_info->major = le16_to_cpu(response.major);
>  	fw_info->minor = le16_to_cpu(response.minor);
>  
> -	strncpy(fw_info->firmware_tag, response.firmware_tag,
> -		GB_FIRMWARE_TAG_MAX_SIZE);
> +	strscpy_pad(fw_info->firmware_tag, response.firmware_tag);
>  
>  	/*
>  	 * The firmware-tag should be NULL terminated, otherwise throw error but
> @@ -153,7 +152,7 @@ static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
>  	}
>  
>  	request.load_method = load_method;
> -	strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> +	strscpy_pad(request.firmware_tag, tag);
>  
>  	/*
>  	 * The firmware-tag should be NULL terminated, otherwise throw error and
> @@ -249,8 +248,7 @@ static int fw_mgmt_backend_fw_version_operation(struct fw_mgmt *fw_mgmt,
>  	struct gb_fw_mgmt_backend_fw_version_response response;
>  	int ret;
>  
> -	strncpy(request.firmware_tag, fw_info->firmware_tag,
> -		GB_FIRMWARE_TAG_MAX_SIZE);
> +	strscpy_pad(request.firmware_tag, fw_info->firmware_tag);
>  
>  	/*
>  	 * The firmware-tag should be NULL terminated, otherwise throw error and
> @@ -303,13 +301,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_pad(request.firmware_tag, tag);
>  
>  	/*
>  	 * 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
> 
>

Link: https://github.com/KSPP/linux/issues/90 [1]

Thanks
Justin

Re: [PATCH 3/3] [v2] staging: greybus: change strncpy() to strscpy_pad()

[Dan Carpenter]

On Mon, Apr 08, 2024 at 09:48:11PM +0200, 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, and I only
> get a warning for one of the four related strncpy()s, so I'm not
> sure what's going on.
> 
> Change all four to strscpy_pad(), which is the safest replacement here,
> as it avoids ending up with uninitialized stack data in the tag name.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2
>  - use strscpy_pad()
>  - use two-argument form
>  - change all four instances, not just the one that produced the warning

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH 1/3] [v2] staging: rts5208: replace weird strncpy() with memcpy()

[Justin Stitt]

Hi,

On Mon, Apr 08, 2024 at 09:48:09PM +0200, 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);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The code originally had a memcpy() that would overread the source string,
> and commit 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
> fixed this but introduced the warning about truncation in the process.
> 
> As Dan points out, the final space in the inquiry_string always gets
> cut off, so remove it here for clarity, leaving exactly the 28 non-NUL
> characters that can get copied into the output. In the 'pro_formatter_flag'
> this is followed by another 20 bytes from the 'formatter_inquiry_str'
> array, but there the output never contains a NUL-termination, and the
> length is known, so memcpy() is the more logical choice.
> 
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/lkml/695be581-548f-4e5e-a211-5f3b95568e77@moroto.mountain/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: remove unneeded space byte from input string for clarity,
>     rework changelog text
> ---
>  drivers/staging/rts5208/rtsx_scsi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
> index 08bd768ad34d..c27cffb9ad8f 100644
> --- a/drivers/staging/rts5208/rtsx_scsi.c
> +++ b/drivers/staging/rts5208/rtsx_scsi.c
> @@ -463,10 +463,10 @@ static unsigned char formatter_inquiry_str[20] = {
>  static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
>  {
>  	unsigned int lun = SCSI_LUN(srb);
> -	char *inquiry_default = (char *)"Generic-xD/SD/M.S.      1.00 ";
> -	char *inquiry_sdms =    (char *)"Generic-SD/MemoryStick  1.00 ";
> -	char *inquiry_sd =      (char *)"Generic-SD/MMC          1.00 ";
> -	char *inquiry_ms =      (char *)"Generic-MemoryStick     1.00 ";
> +	char *inquiry_default = (char *)"Generic-xD/SD/M.S.      1.00";
> +	char *inquiry_sdms =    (char *)"Generic-SD/MemoryStick  1.00";
> +	char *inquiry_sd =      (char *)"Generic-SD/MMC          1.00";
> +	char *inquiry_ms =      (char *)"Generic-MemoryStick     1.00";
>  	char *inquiry_string;
>  	unsigned char sendbytes;
>  	unsigned char *buf;
> @@ -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 must say I am not the biggest fan of manual string management with raw
pointer offsets. I wonder if scnprintf() could achieve your goal here of
combining inquiry_buf with inquiry_string into buf (perhaps utilizing
%.*s format specifier).

With that being said, I am just a casual reader of this code and I
really don't know much about the expected behavior of `buf`
(NUL-terminated, NUL-padded, etc) or even what the next line buf[4]=0x33
does.

Your patch looks good and removes an instance of strncpy which helps
towards [1].

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

>  		if (pro_formatter_flag) {
>  			/* Additional Length */
>  			buf[4] = 0x33;
> -- 
> 2.39.2
> 
>

[1]: https://github.com/KSPP/linux/issues/90

Thanks
Justin

Re: [PATCH 1/3] [v2] staging: rts5208: replace weird strncpy() with memcpy()

[Arnd Bergmann]

On Mon, Apr 8, 2024, at 22:28, Justin Stitt wrote:
> On Mon, Apr 08, 2024 at 09:48:09PM +0200, Arnd Bergmann wrote:

>> @@ -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 must say I am not the biggest fan of manual string management with raw
> pointer offsets. I wonder if scnprintf() could achieve your goal here of
> combining inquiry_buf with inquiry_string into buf (perhaps utilizing
> %.*s format specifier).

scnprintf() would be wrong here, since it's not actually a string
but a SCSI HW data structure with both binary and ASCII members
and no NUL termination. It's supposed to be padded with spaces.

> With that being said, I am just a casual reader of this code and I
> really don't know much about the expected behavior of `buf`
> (NUL-terminated, NUL-padded, etc) or even what the next line buf[4]=0x33
> does.

I believe this sets the length of the buffer to 51 bytes when
pro_formatter_flag is set, overriding the 0x1f (31 bytes) for the
normal length.

The root of the problem here is that the driver emulates a SCSI
command set on a card reader for both SD cards and memory sticks
instead of using the existing abstractions from
drivers/{mmc,memstick}.

Apparently drivers/misc/cardreader/rtsx_pcr.c does this the
right way for all later devices (rts5209 and up) but is
not compatible with this variant of the hardware.

    Arnd

Re: [PATCH 1/3] [v2] staging: rts5208: replace weird strncpy() with memcpy()

[Dan Carpenter]

On Mon, Apr 08, 2024 at 09:48:09PM +0200, 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);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The code originally had a memcpy() that would overread the source string,
> and commit 88a5b39b69ab ("staging/rts5208: Fix read overflow in memcpy")
> fixed this but introduced the warning about truncation in the process.
> 
> As Dan points out, the final space in the inquiry_string always gets
> cut off, so remove it here for clarity, leaving exactly the 28 non-NUL
> characters that can get copied into the output. In the 'pro_formatter_flag'
> this is followed by another 20 bytes from the 'formatter_inquiry_str'
> array, but there the output never contains a NUL-termination, and the
> length is known, so memcpy() is the more logical choice.
> 
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/lkml/695be581-548f-4e5e-a211-5f3b95568e77@moroto.mountain/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: remove unneeded space byte from input string for clarity,
>     rework changelog text

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter