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@xxxxxxxxxxxxxxx > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> Yup, these all look correct. I can confirm the open-coded "16" matches the now-used sizeof(). Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> -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@xxxxxxxxxx> > > -- Kees Cook
