| Message ID | 20231023-strncpy-drivers-scsi-ch-c-v1-1-dc67ba8075a3@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: ch: replace deprecated strncpy with strscpy | expand |
On Mon, Oct 23, 2023 at 08:20:14PM +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. > > These labels get copied out to the user so lets make sure they are > NUL-terminated and NUL-padded. > > vparams is already memset to 0 so we don't need to do any NUL-padding > (like what strncpy() is doing). > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Let's also opt to use the more idiomatic strscpy() usage of: > (dest, src, sizeof(dest)) as this more closely ties the destination > buffer to the length. > > 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> Yup, these all look correct. I can confirm the open-coded "16" matches the now-used sizeof(). Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > drivers/scsi/ch.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c > index cb0a399be1cc..2b864061e073 100644 > --- a/drivers/scsi/ch.c > +++ b/drivers/scsi/ch.c > @@ -659,19 +659,23 @@ static long ch_ioctl(struct file *file, > memset(&vparams,0,sizeof(vparams)); > if (ch->counts[CHET_V1]) { > vparams.cvp_n1 = ch->counts[CHET_V1]; > - strncpy(vparams.cvp_label1,vendor_labels[0],16); > + strscpy(vparams.cvp_label1, vendor_labels[0], > + sizeof(vparams.cvp_label1)); > } > if (ch->counts[CHET_V2]) { > vparams.cvp_n2 = ch->counts[CHET_V2]; > - strncpy(vparams.cvp_label2,vendor_labels[1],16); > + strscpy(vparams.cvp_label2, vendor_labels[1], > + sizeof(vparams.cvp_label2)); > } > if (ch->counts[CHET_V3]) { > vparams.cvp_n3 = ch->counts[CHET_V3]; > - strncpy(vparams.cvp_label3,vendor_labels[2],16); > + strscpy(vparams.cvp_label3, vendor_labels[2], > + sizeof(vparams.cvp_label3)); > } > if (ch->counts[CHET_V4]) { > vparams.cvp_n4 = ch->counts[CHET_V4]; > - strncpy(vparams.cvp_label4,vendor_labels[3],16); > + strscpy(vparams.cvp_label4, vendor_labels[3], > + sizeof(vparams.cvp_label4)); > } > if (copy_to_user(argp, &vparams, sizeof(vparams))) > return -EFAULT; > > --- > base-commit: 9c5d00cb7b6bbc5a7965d9ab7d223b5402d1f02c > change-id: 20231023-strncpy-drivers-scsi-ch-c-23b1cdac43cc > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > >
Justin, > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. Applied to 6.8/scsi-staging, thanks!
On Mon, 23 Oct 2023 20:20:14 +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. > > These labels get copied out to the user so lets make sure they are > NUL-terminated and NUL-padded. > > [...] Applied to 6.8/scsi-queue, thanks! [1/1] scsi: ch: replace deprecated strncpy with strscpy https://git.kernel.org/mkp/scsi/c/dc7a7f10e673
diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c index cb0a399be1cc..2b864061e073 100644 --- a/drivers/scsi/ch.c +++ b/drivers/scsi/ch.c @@ -659,19 +659,23 @@ static long ch_ioctl(struct file *file, memset(&vparams,0,sizeof(vparams)); if (ch->counts[CHET_V1]) { vparams.cvp_n1 = ch->counts[CHET_V1]; - strncpy(vparams.cvp_label1,vendor_labels[0],16); + strscpy(vparams.cvp_label1, vendor_labels[0], + sizeof(vparams.cvp_label1)); } if (ch->counts[CHET_V2]) { vparams.cvp_n2 = ch->counts[CHET_V2]; - strncpy(vparams.cvp_label2,vendor_labels[1],16); + strscpy(vparams.cvp_label2, vendor_labels[1], + sizeof(vparams.cvp_label2)); } if (ch->counts[CHET_V3]) { vparams.cvp_n3 = ch->counts[CHET_V3]; - strncpy(vparams.cvp_label3,vendor_labels[2],16); + strscpy(vparams.cvp_label3, vendor_labels[2], + sizeof(vparams.cvp_label3)); } if (ch->counts[CHET_V4]) { vparams.cvp_n4 = ch->counts[CHET_V4]; - strncpy(vparams.cvp_label4,vendor_labels[3],16); + strscpy(vparams.cvp_label4, vendor_labels[3], + sizeof(vparams.cvp_label4)); } if (copy_to_user(argp, &vparams, sizeof(vparams))) return -EFAULT;
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. These labels get copied out to the user so lets make sure they are NUL-terminated and NUL-padded. vparams is already memset to 0 so we don't need to do any NUL-padding (like what strncpy() is doing). Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Let's also opt to use the more idiomatic strscpy() usage of: (dest, src, sizeof(dest)) as this more closely ties the destination buffer to the length. 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/scsi/ch.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) --- base-commit: 9c5d00cb7b6bbc5a7965d9ab7d223b5402d1f02c change-id: 20231023-strncpy-drivers-scsi-ch-c-23b1cdac43cc Best regards, -- Justin Stitt <justinstitt@google.com>