[PATCH] scsi: target: core: replace deprecated strncpy with strscpy

[PATCH] scsi: target: core: replace deprecated strncpy with strscpy

[Justin Stitt]

strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

We expect p_buf to be NUL-terminated based on the callsites using these
transport_dump_* methods because they use the destination buf with C-string
APIs like strlen() and sprintf().

		memset(buf, 0, VPD_TMP_BUF_SIZE);			\
		transport_dump_vpd_ident_type(vpd, buf, VPD_TMP_BUF_SIZE); \
		if (len + strlen(buf) >= PAGE_SIZE)			\
			break;						\
		len += sprintf(page+len, "%s", buf);			\

We also do not require the NUL-padding behavior that strncpy() provides
because we are manually setting the entire buffer to NUL, rendering any
future padding redundant.

Let's use strscpy() as it guarantees NUL-termination and doesn't
NUL-pad ( and isn't deprecated :>] ). Note that we can't use the more
idiomatic strscpy() usage of strscpy(dest, src, sizeof(dest)) because
the size of the destination buffer is not known to the compiler. We also
can't use the new 2-arg version of strscpy() from Commit e6584c3964f2f
("string: Allow 2-argument strscpy()")

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/target/target_core_transport.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 73d0d6133ac8..3311eb87df6d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1112,7 +1112,7 @@ void transport_dump_vpd_proto_id(
 	}
 
 	if (p_buf)
-		strncpy(p_buf, buf, p_buf_len);
+		strscpy(p_buf, buf, p_buf_len);
 	else
 		pr_debug("%s", buf);
 }
@@ -1162,7 +1162,7 @@ int transport_dump_vpd_assoc(
 	}
 
 	if (p_buf)
-		strncpy(p_buf, buf, p_buf_len);
+		strscpy(p_buf, buf, p_buf_len);
 	else
 		pr_debug("%s", buf);
 
@@ -1222,7 +1222,7 @@ int transport_dump_vpd_ident_type(
 	if (p_buf) {
 		if (p_buf_len < strlen(buf)+1)
 			return -EINVAL;
-		strncpy(p_buf, buf, p_buf_len);
+		strscpy(p_buf, buf, p_buf_len);
 	} else {
 		pr_debug("%s", buf);
 	}
@@ -1276,7 +1276,7 @@ int transport_dump_vpd_ident(
 	}
 
 	if (p_buf)
-		strncpy(p_buf, buf, p_buf_len);
+		strscpy(p_buf, buf, p_buf_len);
 	else
 		pr_debug("%s", buf);
 

---
base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400
change-id: 20240318-strncpy-drivers-target-target_core_transport-c-1950554ec04e

Best regards,
--
Justin Stitt <justinstitt@google.com>

Re: [PATCH] scsi: target: core: replace deprecated strncpy with strscpy

[Kees Cook]

On Mon, Mar 18, 2024 at 10:07:50PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> We expect p_buf to be NUL-terminated based on the callsites using these
> transport_dump_* methods because they use the destination buf with C-string
> APIs like strlen() and sprintf().
> 
> 		memset(buf, 0, VPD_TMP_BUF_SIZE);			\
> 		transport_dump_vpd_ident_type(vpd, buf, VPD_TMP_BUF_SIZE); \
> 		if (len + strlen(buf) >= PAGE_SIZE)			\
> 			break;						\
> 		len += sprintf(page+len, "%s", buf);			\
> 
> We also do not require the NUL-padding behavior that strncpy() provides
> because we are manually setting the entire buffer to NUL, rendering any
> future padding redundant.
> 
> Let's use strscpy() as it guarantees NUL-termination and doesn't
> NUL-pad ( and isn't deprecated :>] ). Note that we can't use the more
> idiomatic strscpy() usage of strscpy(dest, src, sizeof(dest)) because
> the size of the destination buffer is not known to the compiler. We also
> can't use the new 2-arg version of strscpy() from Commit e6584c3964f2f
> ("string: Allow 2-argument strscpy()")
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Hm, this actually fixes potential over-reads and potential memory content
exposures (in the face of malicious/weird hardware) since p_buf_len
appears to always be sizeof(p_buf) in callers, which means the old use
of strncpy() could have left the string unterminated.

In practice I would assume it's not a problem, but, for example, here's
a place where the 254 p_buf_len could run out when doing the sprintf:

#define VPD_TMP_BUF_SIZE                      254
...
#define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN       254
...
struct t10_vpd {
        unsigned char device_identifier[INQUIRY_VPD_DEVICE_IDENTIFIER_LEN];
	...
};
...
int transport_dump_vpd_ident(..., unsigned char *p_buf, int p_buf_len)
{
	...
        unsigned char buf[VPD_TMP_BUF_SIZE];
	...
                snprintf(buf, sizeof(buf),
                        "T10 VPD ASCII Device Identifier: %s\n",
                        &vpd->device_identifier[0]);
	...
        if (p_buf)
                strncpy(p_buf, buf, p_buf_len);	// may write 254 chars and no NUL
	...
}
...
        unsigned char buf[VPD_TMP_BUF_SIZE];
	...
	memset(buf, 0, VPD_TMP_BUF_SIZE);
        transport_dump_vpd_ident_type(vpd, buf, VPD_TMP_BUF_SIZE);
        if (len + strlen(buf) >= PAGE_SIZE)
		break;
	len += sprintf(page+len, "%s", buf);	// may expose stack memory following "buf"


So, yes, please!

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> Note: build-tested only.
> 
> Found with: $ rg "strncpy\("
> ---
>  drivers/target/target_core_transport.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 73d0d6133ac8..3311eb87df6d 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1112,7 +1112,7 @@ void transport_dump_vpd_proto_id(
>  	}
>  
>  	if (p_buf)
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);
>  	else
>  		pr_debug("%s", buf);
>  }
> @@ -1162,7 +1162,7 @@ int transport_dump_vpd_assoc(
>  	}
>  
>  	if (p_buf)
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);
>  	else
>  		pr_debug("%s", buf);
>  
> @@ -1222,7 +1222,7 @@ int transport_dump_vpd_ident_type(
>  	if (p_buf) {
>  		if (p_buf_len < strlen(buf)+1)
>  			return -EINVAL;
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);

>  	} else {
>  		pr_debug("%s", buf);
>  	}
> @@ -1276,7 +1276,7 @@ int transport_dump_vpd_ident(
>  	}
>  
>  	if (p_buf)
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);
>  	else
>  		pr_debug("%s", buf);
>  
> 
> ---
> base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400
> change-id: 20240318-strncpy-drivers-target-target_core_transport-c-1950554ec04e
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
> 
> 

-- 
Kees Cook

Re: [PATCH] scsi: target: core: replace deprecated strncpy with strscpy

[Kees Cook]

On Tue, Mar 19, 2024 at 07:23:43PM -0700, Kees Cook wrote:
> Hm, this actually fixes potential over-reads and potential memory content
> exposures (in the face of malicious/weird hardware) since p_buf_len
> appears to always be sizeof(p_buf) in callers, which means the old use
> of strncpy() could have left the string unterminated.
> 
> In practice I would assume it's not a problem, but, for example, here's
> a place where the 254 p_buf_len could run out when doing the sprintf:
> 
> #define VPD_TMP_BUF_SIZE                      254
> ...
> #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN       254
> ...
> struct t10_vpd {
>         unsigned char device_identifier[INQUIRY_VPD_DEVICE_IDENTIFIER_LEN];
> 	...
> };
> ...
> int transport_dump_vpd_ident(..., unsigned char *p_buf, int p_buf_len)
> {
> 	...
>         unsigned char buf[VPD_TMP_BUF_SIZE];
> 	...
>                 snprintf(buf, sizeof(buf),
>                         "T10 VPD ASCII Device Identifier: %s\n",
>                         &vpd->device_identifier[0]);
> 	...
>         if (p_buf)
>                 strncpy(p_buf, buf, p_buf_len);	// may write 254 chars and no NUL

Wait, no, it's safe; I got confused by the double buffers. The
snprintf() will always NUL-terminate, so "buf" copied into p_buf will
always be NUL terminated.

So, nevermind! Regardless, still a good conversion. Thank you!

-Kees

-- 
Kees Cook