Skip to contentSkip to conversation

scsi: ibmvscsis: Fix a stringop-overflow warning

PATCH Superseded Laura Abbott 3 participants 4 messages Sep 10, 2018 – Sep 11, 2018

Subsystems (2)
SCSI IBM Power Virtual SCSI Device Target Driver

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Laura Abbott

author
#0 lore raw

PATCH 1/1 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 using strlcat which correctly accounts for lengths that are equal to the destination buffer.

Signed-off-by Laura Abbott <[email protected]>
---
 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 fac377320158..53d344d8dc43 100644
@@ -3475,7 +3475,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
 		vscsi->dds.window[REMOTE].liobn);
 
 	strcpy(vscsi->eye, "VSCSI ");
-	strncat(vscsi->eye, vdev->name, MAX_EYE);
+	strlcat(vscsi->eye, vdev->name, MAX_EYE);
 
 	vscsi->dds.unit_id = vdev->unit_address;
 	strncpy(vscsi->dds.partition_name, partition_name,
Signature · 1 line
2.17.1
#1 lore raw
Quoted 21 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 using strlcat which correctly accounts for lengths that are equal to the destination buffer.

Signed-off-by Laura Abbott <[email protected]>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +0 -0
index fac377320158..53d344d8dc43 100644

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c static int ibmvscsis_probe(struct vio_dev *vdev, · line 3478

Quoted 5 lines
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +1 -1
@@ -3475,7 +3475,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
                vscsi->dds.window[REMOTE].liobn);
 
        strcpy(vscsi->eye, "VSCSI ");
-       strncat(vscsi->eye, vdev->name, MAX_EYE);
+       strlcat(vscsi->eye, vdev->name, MAX_EYE);

Can this just get switched to snprintf() instead?

snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI%s", vdev->name);

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c static int ibmvscsis_probe(struct vio_dev *vdev, · line 3481

Quoted 2 lines
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +0 -0
        vscsi->dds.unit_id = vdev->unit_address;
        strncpy(vscsi->dds.partition_name, partition_name,

And does this strncpy need a NUL-termination and/or trailing padding?

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#2 lore raw
Quoted 24 lines · Kees Cook
Quoted 23 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 using strlcat which correctly accounts for lengths that are equal to the destination buffer.

Signed-off-by Laura Abbott <[email protected]>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +0 -0
index fac377320158..53d344d8dc43 100644
@@ -3475,7 +3475,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
                vscsi->dds.window[REMOTE].liobn);
Quoted 6 lines

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c static int ibmvscsis_probe(struct vio_dev *vdev, · line 3478

Quoted 3 lines
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +1 -1
        strcpy(vscsi->eye, "VSCSI ");
-       strncat(vscsi->eye, vdev->name, MAX_EYE);
+       strlcat(vscsi->eye, vdev->name, MAX_EYE);

Can this just get switched to snprintf() instead?

snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI%s", vdev->name);

Either works for me.

Quoted 5 lines

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c static int ibmvscsis_probe(struct vio_dev *vdev, · line 3481

Quoted 3 lines
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +0 -0
        vscsi->dds.unit_id = vdev->unit_address;
        strncpy(vscsi->dds.partition_name, partition_name,

And does this strncpy need a NUL-termination and/or trailing padding?

Yes, good catch, it does need NUL-termination.

-Bryant

author
#3 lore raw
Quoted 26 lines · Bryant G. Ly
Quoted 24 lines · Kees Cook
Quoted 23 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 using strlcat which correctly accounts for lengths that are equal to the destination buffer.

Signed-off-by Laura Abbott <[email protected]>
---
  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +0 -0
index fac377320158..53d344d8dc43 100644
@@ -3475,7 +3475,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev,
                 vscsi->dds.window[REMOTE].liobn);
Quoted 9 lines
Quoted 6 lines

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c static int ibmvscsis_probe(struct vio_dev *vdev, · line 3478

Quoted 3 lines
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +1 -1
         strcpy(vscsi->eye, "VSCSI ");
-       strncat(vscsi->eye, vdev->name, MAX_EYE);
+       strlcat(vscsi->eye, vdev->name, MAX_EYE);

Can this just get switched to snprintf() instead?

snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI%s", vdev->name);

Either works for me.

I'll switch to the snprintf since that's cleaner.

Quoted 7 lines
Quoted 5 lines

drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c static int ibmvscsis_probe(struct vio_dev *vdev, · line 3481

Quoted 3 lines
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c +0 -0
         vscsi->dds.unit_id = vdev->unit_address;
         strncpy(vscsi->dds.partition_name, partition_name,

And does this strncpy need a NUL-termination and/or trailing padding?

Yes, good catch, it does need NUL-termination.

I can send that cleanup in a second patch.

Quoted 3 lines

-Bryant