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@xxxxxxxxxxxxxxx
> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
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@xxxxxxxxxxxx>
-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@xxxxxxxxxx>
>
>
--
Kees Cook
