Message ID | 20240318-strncpy-drivers-target-target_core_transport-c-v1-1-a086b9a0d639@google.com |
---|---|
State | New |
Headers | show |
Series | scsi: target: core: replace deprecated strncpy with strscpy | expand |
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> > >
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
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);
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(-) --- base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400 change-id: 20240318-strncpy-drivers-target-target_core_transport-c-1950554ec04e Best regards, -- Justin Stitt <justinstitt@google.com>