diff mbox series

scsi: message: fusion: replace deprecated strncpy with strscpy_pad

Message ID 20230927-strncpy-drivers-message-fusion-mptctl-c-v1-1-bb2eddc1743c@google.com (mailing list archive)
State Mainlined
Commit 4280a0a70170f60df83ddb46eaee7bbbe88622d9
Headers show
Series scsi: message: fusion: replace deprecated strncpy with strscpy_pad | expand

Commit Message

Justin Stitt Sept. 27, 2023, 4:06 a.m. UTC
`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

Since all these structs are copied out to userspace let's keep them
NUL-padded by using `strscpy_pad` which guarantees NUL-termination of
the destination buffer while also providing the NUL-padding behavior
that strncpy has.

Let's also opt to use the more idiomatic strscpy usage of:
`dest, src, sizeof(dest)` in cases where the compiler can determine the
size of the destination buffer. Do this for all cases of strscpy...() in
this file.

To be abundantly sure we don't leak stack data out to user space let's
also change a strscpy to strscpy_pad. This strscpy was introduced in
Commit dbe37c71d1246ec2 ("scsi: message: fusion: Replace all
non-returning strlcpy() with strscpy()")

Note that since we are creating these structs with a copy_from_user()
and modifying fields and then copying back out to the user it is
probably OK not to explicitly NUL-pad everything as any data leak is
probably just data from the user themselves. If this is too eager, let's
opt for `strscpy` which is still in the spirit of removing deprecated
strncpy usage treewide.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.
---
 drivers/message/fusion/mptctl.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)


---
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
change-id: 20230927-strncpy-drivers-message-fusion-mptctl-c-5e7558cd93ab

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Kees Cook Oct. 2, 2023, 5:41 p.m. UTC | #1
On Wed, Sep 27, 2023 at 04:06:09AM +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.
> 
> Since all these structs are copied out to userspace let's keep them
> NUL-padded by using `strscpy_pad` which guarantees NUL-termination of
> the destination buffer while also providing the NUL-padding behavior
> that strncpy has.
> 
> Let's also opt to use the more idiomatic strscpy usage of:
> `dest, src, sizeof(dest)` in cases where the compiler can determine the
> size of the destination buffer. Do this for all cases of strscpy...() in
> this file.
> 
> To be abundantly sure we don't leak stack data out to user space let's
> also change a strscpy to strscpy_pad. This strscpy was introduced in
> Commit dbe37c71d1246ec2 ("scsi: message: fusion: Replace all
> non-returning strlcpy() with strscpy()")
> 
> Note that since we are creating these structs with a copy_from_user()
> and modifying fields and then copying back out to the user it is
> probably OK not to explicitly NUL-pad everything as any data leak is
> probably just data from the user themselves. If this is too eager, let's
> opt for `strscpy` which is still in the spirit of removing deprecated
> strncpy usage treewide.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Agreed -- this looks more robust and readable. Thanks!

Reviewed-by: Kees Cook <keescook@chromium.org>
Martin K. Petersen Oct. 10, 2023, 1:29 a.m. UTC | #2
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.7/scsi-staging, thanks!
Martin K. Petersen Oct. 13, 2023, 9:03 p.m. UTC | #3
On Wed, 27 Sep 2023 04:06:09 +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.
> 
> Since all these structs are copied out to userspace let's keep them
> NUL-padded by using `strscpy_pad` which guarantees NUL-termination of
> the destination buffer while also providing the NUL-padding behavior
> that strncpy has.
> 
> [...]

Applied to 6.7/scsi-queue, thanks!

[1/1] scsi: message: fusion: replace deprecated strncpy with strscpy_pad
      https://git.kernel.org/mkp/scsi/c/4280a0a70170
diff mbox series

Patch

diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index dd028df4b283..9f3999750c23 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -1328,8 +1328,8 @@  mptctl_getiocinfo (MPT_ADAPTER *ioc, unsigned long arg, unsigned int data_size)
 
 	/* Set the Version Strings.
 	 */
-	strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, MPT_IOCTL_VERSION_LENGTH);
-	karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';
+	strscpy_pad(karg->driverVersion, MPT_LINUX_PACKAGE_NAME,
+		    sizeof(karg->driverVersion));
 
 	karg->busChangeEvent = 0;
 	karg->hostId = ioc->pfacts[port].PortSCSIID;
@@ -1493,10 +1493,8 @@  mptctl_readtest (MPT_ADAPTER *ioc, unsigned long arg)
 #else
 	karg.chip_type = ioc->pcidev->device;
 #endif
-	strncpy (karg.name, ioc->name, MPT_MAX_NAME);
-	karg.name[MPT_MAX_NAME-1]='\0';
-	strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
-	karg.product[MPT_PRODUCT_LENGTH-1]='\0';
+	strscpy_pad(karg.name, ioc->name, sizeof(karg.name));
+	strscpy_pad(karg.product, ioc->prod_name, sizeof(karg.product));
 
 	/* Copy the data from kernel memory to user memory
 	 */
@@ -2394,7 +2392,7 @@  mptctl_hp_hostinfo(MPT_ADAPTER *ioc, unsigned long arg, unsigned int data_size)
 	cfg.dir = 0;	/* read */
 	cfg.timeout = 10;
 
-	strncpy(karg.serial_number, " ", 24);
+	strscpy_pad(karg.serial_number, " ", sizeof(karg.serial_number));
 	if (mpt_config(ioc, &cfg) == 0) {
 		if (cfg.cfghdr.hdr->PageLength > 0) {
 			/* Issue the second config page request */
@@ -2408,8 +2406,9 @@  mptctl_hp_hostinfo(MPT_ADAPTER *ioc, unsigned long arg, unsigned int data_size)
 				if (mpt_config(ioc, &cfg) == 0) {
 					ManufacturingPage0_t *pdata = (ManufacturingPage0_t *) pbuf;
 					if (strlen(pdata->BoardTracerNumber) > 1) {
-						strscpy(karg.serial_number,
-							pdata->BoardTracerNumber, 24);
+						strscpy_pad(karg.serial_number,
+							pdata->BoardTracerNumber,
+							sizeof(karg.serial_number));
 					}
 				}
 				dma_free_coherent(&ioc->pcidev->dev,
@@ -2456,7 +2455,7 @@  mptctl_hp_hostinfo(MPT_ADAPTER *ioc, unsigned long arg, unsigned int data_size)
 		}
 	}
 
-	/* 
+	/*
 	 * Gather ISTWI(Industry Standard Two Wire Interface) Data
 	 */
 	if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {