Skip to contentSkip to conversation

String fixups in ibmvscsi

PATCH Applied Laura Abbott 5 participants 7 messages Sep 11, 2018 – Sep 18, 2018

Subsystems (3)
SCSI SCSI target IBM Power Virtual SCSI Device Target Driver
lore
Share & tools
lore

Laura Abbott

author
#0 lore raw

v3 cover String fixups in ibmvscsi

Hi,

This is (hopefully) the last version of the string fixups found in the ibmvscsi driver.

Laura Abbott (2):
scsi: ibmvscsis: Fix a stringop-overflow warning
scsi: ibmvscsis: Ensure partition name is properly NUL terminated

 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v3 1/2 scsi: ibmvscsis: Fix a stringop-overflow warning

There's currently a warning about string overflow with strncat:

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c: In function 'ibmvscsis_probe':
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c:3479:2: error: 'strncat' specified bound 64 equals destination size [-Werror=stringop-overflow=]
strncat(vscsi->eye, vdev->name, MAX_EYE); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Switch to a single snprintf instead of a strcpy + strcat to handle this cleanly.

Signed-off-by Laura Abbott <[email protected]>

---
v3: Make sure there is an extra space in the partition name

---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +1 -2
index fac377320158..b3a029ad07cd 100644
@@ -3474,8 +3474,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
 		vscsi->dds.window[LOCAL].liobn,
 		vscsi->dds.window[REMOTE].liobn);
 
-	strcpy(vscsi->eye, "VSCSI ");
-	strncat(vscsi->eye, vdev->name, MAX_EYE);
+	snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI %s", vdev->name);
 
 	vscsi->dds.unit_id = vdev->unit_address;
 	strncpy(vscsi->dds.partition_name, partition_name,
Signature · 1 line
2.17.1
Reviewed-by on v3 1/2
#2 lore raw

Re: [PATCHv3 1/2] scsi: ibmvscsis: Fix a stringop-overflow warning

Quoted 15 lines · Laura Abbott

There's currently a warning about string overflow with strncat:

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c: In function 'ibmvscsis_probe':
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c:3479:2: error: 'strncat' specified bound 64 equals destination size [-Werror=stringop-overflow=]
strncat(vscsi->eye, vdev->name, MAX_EYE); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Switch to a single snprintf instead of a strcpy + strcat to handle this cleanly.

Quoted 7 lines
Signed-off-by Laura Abbott <[email protected]>

---
v3: Make sure there is an extra space in the partition name
---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +--

1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by Bryant G. Ly [email protected]

I sent a PR to update my email from [email protected] a week ago.

-Bryant

author
#3 lore raw

PATCH v2 2/2 scsi: ibmvscsis: Ensure partition name is properly NUL terminated

In reply to Laura Abbott

While reviewing another part of the code, Kees noticed that the strncpy of the partition name might not always be NUL terminated. Switch to using strscpy which does this safely.

Reported-by Kees Cook <[email protected]>
Signed-off-by Laura Abbott <[email protected]>

---
v2: Switch to strscpy instead of just strlcpy

---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +1 -1
index b3a029ad07cd..f42a619198c4 100644
@@ -3477,7 +3477,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
 	snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI %s", vdev->name);
 
 	vscsi->dds.unit_id = vdev->unit_address;
-	strncpy(vscsi->dds.partition_name, partition_name,
+	strscpy(vscsi->dds.partition_name, partition_name,
 		sizeof(vscsi->dds.partition_name));
 	vscsi->dds.partition_num = partition_number;
Signature · 1 line
2.17.1
Reviewed-by on v2 2/2
#4 lore raw

Re: [PATCHv2 2/2] scsi: ibmvscsis: Ensure partition name is properly NUL terminated

Quoted 7 lines · Laura Abbott

While reviewing another part of the code, Kees noticed that the strncpy of the partition name might not always be NUL terminated. Switch to using strscpy which does this safely.

Reported-by Kees Cook <[email protected]>
Signed-off-by Laura Abbott <[email protected]>
Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 22 lines

---
v2: Switch to strscpy instead of just strlcpy

---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +1 -2
index b3a029ad07cd..f42a619198c4 100644
@@ -3477,7 +3477,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
        snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI %s", vdev->name);
 
        vscsi->dds.unit_id = vdev->unit_address;
-       strncpy(vscsi->dds.partition_name, partition_name,
+       strscpy(vscsi->dds.partition_name, partition_name,
                sizeof(vscsi->dds.partition_name));
        vscsi->dds.partition_num = partition_number;
 
--

2.17.1

Signature · 2 lines
Kees Cook
Pixel Security
Reviewed-by Bryant G. Ly on v2 2/2
#5 lore raw

Re: [PATCHv2 2/2] scsi: ibmvscsis: Ensure partition name is properly NUL terminated

In reply to Laura Abbott
Quoted 8 lines · Laura Abbott

While reviewing another part of the code, Kees noticed that the strncpy of the partition name might not always be NUL terminated. Switch to using strscpy which does this safely.

Quoted 9 lines
Reported-by Kees Cook <[email protected]>
Signed-off-by Laura Abbott <[email protected]>
Reviewed-by Kees Cook <[email protected]>

---
v2: Switch to strscpy instead of just strlcpy
---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by Bryant G. Ly [email protected]

I sent a PR to update my email from [email protected] a week ago.

-Bryant

Applied
#6 lore raw
In reply to Laura Abbott

Laura,

Quoted 2 lines

This is (hopefully) the last version of the string fixups found in the ibmvscsi driver.

Applied to 4.19/scsi-fixes, thank you!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering