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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
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 <[email protected]>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
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 <[email protected]>
-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 <[email protected]>
>
>
--
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