Skip to contentSkip to conversation

scsi: qedf: Avoid reading past end of buffer

PATCH Applied Kees Cook 4 participants 5 messages May 5, 2017 – May 9, 2017

Subsystems (2)
SCSI QLogic QL41xxx FCoE driver
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 scsi: qedf: Avoid reading past end of buffer

Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, use strncpy() which will fill the trailing bytes with zeros.

This was found with the future CONFIG_FORTIFY_SOURCE feature.

Cc Daniel Micay <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 drivers/scsi/qedf/qedf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qedf/qedf_main.c +1 -1
index cceddd995a4b..a5c97342fd5d 100644
@@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
 	slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
 	slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
 	slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
-	memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+	strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
 	rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
 	if (rc) {
 		QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
#1 lore raw

drivers/scsi/qedf/qedf_main.c static int __qedf_probe(struct pci_dev *pdev, int mode) · line 2901

Quoted 13 lines · Kees Cook
drivers/scsi/qedf/qedf_main.c +1 -1
index cceddd995a4b..a5c97342fd5d 100644
@@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
 	slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
 	slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
 	slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
-	memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+	strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
 	rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
 	if (rc) {
 		QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");

Hello Kees,

Although this patch looks fine to me, isn't strlcpy() preferred over strncpy()?

Bart.

author
#2 lore raw
Quoted 17 lines · Bart Van Assche

drivers/scsi/qedf/qedf_main.c static int __qedf_probe(struct pci_dev *pdev, int mode) · line 2901

Quoted 13 lines · Kees Cook
drivers/scsi/qedf/qedf_main.c +1 -1
index cceddd995a4b..a5c97342fd5d 100644
@@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
      slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
      slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
      slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
-     memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+     strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
      rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
      if (rc) {
              QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");

Hello Kees,

Quoted 1 line

Although this patch looks fine to me, isn't strlcpy() preferred over strncpy()?

strlcpy doesn't zero-pad, so I think strncpy is preferred here, otherwise we may risk leaving portions of the destination buffer filled with uninitialized data, maybe leaking kernel memory contents.

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
Acked-by
#3 lore raw
Quoted 19 lines · Kees Cook
Quoted 17 lines · Bart Van Assche

drivers/scsi/qedf/qedf_main.c static int __qedf_probe(struct pci_dev *pdev, int mode) · line 2901

Quoted 13 lines · Kees Cook
drivers/scsi/qedf/qedf_main.c +1 -1
index cceddd995a4b..a5c97342fd5d 100644
@@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
      slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
      slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
      slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
-     memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+     strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
      rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
      if (rc) {
              QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");

Hello Kees,

Quoted 8 lines
Quoted 1 line

Although this patch looks fine to me, isn't strlcpy() preferred over strncpy()?

strlcpy doesn't zero-pad, so I think strncpy is preferred here, otherwise we may risk leaving portions of the destination buffer filled with uninitialized data, maybe leaking kernel memory contents.

-Kees

I'd agree with strncpy so we zero out the rest of the buffer.

Acked-by Chad Dupuis <[email protected]>
Applied
#4 lore raw
In reply to Kees Cook

Kees,

Quoted 4 lines

Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, use strncpy() which will fill the trailing bytes with zeros.

Applied to 4.12/scsi-fixes, thanks!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering