From: Kees Cook <keescook@chromium.org>
To: Justin Stitt <justinstitt@google.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] scsi: target: core: replace deprecated strncpy with strscpy
Date: Tue, 19 Mar 2024 19:23:43 -0700 [thread overview]
Message-ID: <202403191908.1B6BBA3@keescook> (raw)
In-Reply-To: <20240318-strncpy-drivers-target-target_core_transport-c-v1-1-a086b9a0d639@google.com>
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
next prev parent reply other threads:[~2024-03-20 2:23 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 22:07 [PATCH] scsi: target: core: replace deprecated strncpy with strscpy Justin Stitt
2024-03-20 2:23 ` Kees Cook [this message]
2024-03-20 2:42 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202403191908.1B6BBA3@keescook \
--to=keescook@chromium.org \
--cc=justinstitt@google.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=target-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).