[PATCHv3 0/2] String fixups in ibmvscsi

[PATCHv3 0/2] String fixups in ibmvscsi

[Laura Abbott]

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(-)

-- 
2.17.1

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

[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.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
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(-)

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index fac377320158..b3a029ad07cd 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -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,
-- 
2.17.1

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

[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 <keescook@chromium.org>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Switch to strscpy instead of just strlcpy
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index b3a029ad07cd..f42a619198c4 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -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

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

[Kees Cook]

On Tue, Sep 11, 2018 at 12:22 PM, Laura Abbott <labbott@redhat.com> wrote:
>
> 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 <keescook@chromium.org>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> v2: Switch to strscpy instead of just strlcpy
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index b3a029ad07cd..f42a619198c4 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -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
>



-- 
Kees Cook
Pixel Security

Re: [PATCHv3 0/2] String fixups in ibmvscsi

[Martin K. Petersen]

Laura,

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

Applied to 4.19/scsi-fixes, thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Ly, Bryant]

> On Sep 11, 2018, at 2:22 PM, Laura Abbott <labbott@redhat.com> wrote:
> 
> 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 <labbott@redhat.com>
> ---
> 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 bly@catalogicsoftware.com

I sent a PR to update my email from bryantly@linux.vnet.ibm.com a week ago.

-Bryant

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

[Bryant Ly]

> On Sep 11, 2018, at 2:22 PM, Laura Abbott <labbott@redhat.com> wrote:
> 
> 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 <keescook@chromium.org>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> 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 bly@catalogicsoftware.com

I sent a PR to update my email from bryantly@linux.vnet.ibm.com a week ago.

-Bryant