| Message ID | 20231030-strncpy-drivers-scsi-ibmvscsi-ibmvfc-c-v1-1-5a4909688435@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: ibmvfc: replace deprecated strncpy with strscpy | expand |
On Mon, Oct 30, 2023 at 07:04:33PM +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 these fields to be NUL-terminated as the property names from > which they are derived are also NUL-terminated. > > Moreover, NUL-padding is not required as our destination buffers are > already NUL-allocated and any future NUL-byte assignments are redundant > (like the ones that strncpy() does). > ibmvfc_probe() -> > | struct ibmvfc_host *vhost; > | struct Scsi_Host *shost; > ... > | shost = scsi_host_alloc(&driver_template, sizeof(*vhost)); > ... **side note: is this a bug? Looks like a type to me ^^^^^** I think this is the "privsize", so *vhost is correct, IIUC. > ... > | vhost = shost_priv(shost); I.e. vhost is a part of the shost allocation > > ... where shost_priv() is: > | static inline void *shost_priv(struct Scsi_Host *shost) > | { > | return (void *)shost->hostdata; > | } > > .. and: > scsi_host_alloc() -> > | shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); As seen here. :) > > And for login_info->..., NUL-padding is also not required as it is > explicitly memset to 0: > | memset(login_info, 0, sizeof(*login_info)); > > 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. > > 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> Yeah, this conversion looks correct to me too. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > drivers/scsi/ibmvscsi/ibmvfc.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c > index ce9eb00e2ca0..e73a39b1c832 100644 > --- a/drivers/scsi/ibmvscsi/ibmvfc.c > +++ b/drivers/scsi/ibmvscsi/ibmvfc.c > @@ -1455,7 +1455,7 @@ static void ibmvfc_gather_partition_info(struct ibmvfc_host *vhost) > > name = of_get_property(rootdn, "ibm,partition-name", NULL); > if (name) > - strncpy(vhost->partition_name, name, sizeof(vhost->partition_name)); > + strscpy(vhost->partition_name, name, sizeof(vhost->partition_name)); > num = of_get_property(rootdn, "ibm,partition-no", NULL); > if (num) > vhost->partition_number = *num; > @@ -1498,13 +1498,15 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost) > login_info->async.va = cpu_to_be64(vhost->async_crq.msg_token); > login_info->async.len = cpu_to_be32(async_crq->size * > sizeof(*async_crq->msgs.async)); > - strncpy(login_info->partition_name, vhost->partition_name, IBMVFC_MAX_NAME); > - strncpy(login_info->device_name, > - dev_name(&vhost->host->shost_gendev), IBMVFC_MAX_NAME); > + strscpy(login_info->partition_name, vhost->partition_name, > + sizeof(login_info->partition_name)); > + > + strscpy(login_info->device_name, > + dev_name(&vhost->host->shost_gendev), sizeof(login_info->device_name)); > > location = of_get_property(of_node, "ibm,loc-code", NULL); > location = location ? location : dev_name(vhost->dev); > - strncpy(login_info->drc_name, location, IBMVFC_MAX_NAME); > + strscpy(login_info->drc_name, location, sizeof(login_info->drc_name)); > } > > /** > > --- > base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa > change-id: 20231030-strncpy-drivers-scsi-ibmvscsi-ibmvfc-c-ccfce3255d58 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
Kees, > On Mon, Oct 30, 2023 at 07:04:33PM +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. > Yeah, this conversion looks correct to me too. > > Reviewed-by: Kees Cook <keescook@chromium.org> Applied to 6.8/scsi-staging, thanks!
On Mon, 30 Oct 2023 19:04:33 +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 these fields to be NUL-terminated as the property names from > which they are derived are also NUL-terminated. > > [...] Applied to 6.8/scsi-queue, thanks! [1/1] scsi: ibmvfc: replace deprecated strncpy with strscpy https://git.kernel.org/mkp/scsi/c/a9baa16b4fc1
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c index ce9eb00e2ca0..e73a39b1c832 100644 --- a/drivers/scsi/ibmvscsi/ibmvfc.c +++ b/drivers/scsi/ibmvscsi/ibmvfc.c @@ -1455,7 +1455,7 @@ static void ibmvfc_gather_partition_info(struct ibmvfc_host *vhost) name = of_get_property(rootdn, "ibm,partition-name", NULL); if (name) - strncpy(vhost->partition_name, name, sizeof(vhost->partition_name)); + strscpy(vhost->partition_name, name, sizeof(vhost->partition_name)); num = of_get_property(rootdn, "ibm,partition-no", NULL); if (num) vhost->partition_number = *num; @@ -1498,13 +1498,15 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost) login_info->async.va = cpu_to_be64(vhost->async_crq.msg_token); login_info->async.len = cpu_to_be32(async_crq->size * sizeof(*async_crq->msgs.async)); - strncpy(login_info->partition_name, vhost->partition_name, IBMVFC_MAX_NAME); - strncpy(login_info->device_name, - dev_name(&vhost->host->shost_gendev), IBMVFC_MAX_NAME); + strscpy(login_info->partition_name, vhost->partition_name, + sizeof(login_info->partition_name)); + + strscpy(login_info->device_name, + dev_name(&vhost->host->shost_gendev), sizeof(login_info->device_name)); location = of_get_property(of_node, "ibm,loc-code", NULL); location = location ? location : dev_name(vhost->dev); - strncpy(login_info->drc_name, location, IBMVFC_MAX_NAME); + strscpy(login_info->drc_name, location, sizeof(login_info->drc_name)); } /**
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 these fields to be NUL-terminated as the property names from which they are derived are also NUL-terminated. Moreover, NUL-padding is not required as our destination buffers are already NUL-allocated and any future NUL-byte assignments are redundant (like the ones that strncpy() does). ibmvfc_probe() -> | struct ibmvfc_host *vhost; | struct Scsi_Host *shost; ... | shost = scsi_host_alloc(&driver_template, sizeof(*vhost)); ... **side note: is this a bug? Looks like a type to me ^^^^^** ... | vhost = shost_priv(shost); ... where shost_priv() is: | static inline void *shost_priv(struct Scsi_Host *shost) | { | return (void *)shost->hostdata; | } .. and: scsi_host_alloc() -> | shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); And for login_info->..., NUL-padding is also not required as it is explicitly memset to 0: | memset(login_info, 0, sizeof(*login_info)); 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. 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/ibmvscsi/ibmvfc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) --- base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa change-id: 20231030-strncpy-drivers-scsi-ibmvscsi-ibmvfc-c-ccfce3255d58 Best regards, -- Justin Stitt <justinstitt@google.com>