[PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings

Kees Cook posted 5 patches 1 year, 10 months ago
drivers/message/fusion/mptsas.c          | 14 +++----
drivers/scsi/mpi3mr/mpi3mr_transport.c   | 14 +++----
drivers/scsi/mpt3sas/mpt3sas_base.c      |  2 +-
drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++----
drivers/scsi/qla2xxx/qla_mr.c            |  6 +--
include/linux/string.h                   | 49 ++++++++++++++++++++++++
lib/strscpy_kunit.c                      | 26 +++++++++++++
7 files changed, 93 insertions(+), 32 deletions(-)
[PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
Posted by Kees Cook 1 year, 10 months ago
Hi,

Another code pattern using the gloriously ambiguous strncpy() function was
turning maybe not-NUL-terminated character arrays into NUL-terminated
strings. In these cases, when strncpy() is replaced with strscpy()
it creates a situation where if the non-terminated string takes up the
entire character array (i.e. it is not terminated) run-time warnings
from CONFIG_FORTIFY_SOURCE will trip, since strscpy() was expecting to
find a NUL-terminated source but checking for the NUL would walk off
the end of the source buffer.

In doing an instrumented build of the kernel to find these cases, it
seems it was almost entirely a code pattern used in the SCSI subsystem,
so the creation of the new helper, memtostr(), can land via the SCSI
tree. And, as it turns out, inappropriate conversions have been happening
for several years now, some of which even moved through strlcpy() first
(and were never noticed either).

This series fixes all 4 of the instances I could find in the SCSI
subsystem.

Thanks,

-Kees

Kees Cook (5):
  string.h: Introduce memtostr() and memtostr_pad()
  scsi: mptfusion: Avoid possible run-time warning with long
    manufacturer strings
  scsi: mpt3sas: Avoid possible run-time warning with long manufacturer
    strings
  scsi: mpi3mr: Avoid possible run-time warning with long manufacturer
    strings
  scsi: qla2xxx: Avoid possible run-time warning with long model_num

 drivers/message/fusion/mptsas.c          | 14 +++----
 drivers/scsi/mpi3mr/mpi3mr_transport.c   | 14 +++----
 drivers/scsi/mpt3sas/mpt3sas_base.c      |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++----
 drivers/scsi/qla2xxx/qla_mr.c            |  6 +--
 include/linux/string.h                   | 49 ++++++++++++++++++++++++
 lib/strscpy_kunit.c                      | 26 +++++++++++++
 7 files changed, 93 insertions(+), 32 deletions(-)

-- 
2.34.1
Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
Posted by Martin K. Petersen 1 year, 9 months ago
Kees,

> This series fixes all 4 of the instances I could find in the SCSI
> subsystem.

Looks OK to me.

Minor nit: I do find it a bit odd to think of a string as "memory".
Maybe that's just because I am so used to always having to distinguish
between fixed length strings and NUL-terminated strings in the storage
protocols and hardware programming interfaces. But both types are
definitely referred to as "strings" colloquially and not so much
"memory".

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering
Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
Posted by Kees Cook 1 year, 9 months ago
On Tue, Apr 09, 2024 at 07:31:49PM -0700, Kees Cook wrote:
> Hi,
> 
> Another code pattern using the gloriously ambiguous strncpy() function was
> turning maybe not-NUL-terminated character arrays into NUL-terminated
> strings. In these cases, when strncpy() is replaced with strscpy()
> it creates a situation where if the non-terminated string takes up the
> entire character array (i.e. it is not terminated) run-time warnings
> from CONFIG_FORTIFY_SOURCE will trip, since strscpy() was expecting to
> find a NUL-terminated source but checking for the NUL would walk off
> the end of the source buffer.
> 
> In doing an instrumented build of the kernel to find these cases, it
> seems it was almost entirely a code pattern used in the SCSI subsystem,
> so the creation of the new helper, memtostr(), can land via the SCSI
> tree. And, as it turns out, inappropriate conversions have been happening
> for several years now, some of which even moved through strlcpy() first
> (and were never noticed either).
> 
> This series fixes all 4 of the instances I could find in the SCSI
> subsystem.

Friendly ping. Can the SCSI tree pick this up, or should I take it
through the hardening tree?

Thanks!

-Kees

> 
> Thanks,
> 
> -Kees
> 
> Kees Cook (5):
>   string.h: Introduce memtostr() and memtostr_pad()
>   scsi: mptfusion: Avoid possible run-time warning with long
>     manufacturer strings
>   scsi: mpt3sas: Avoid possible run-time warning with long manufacturer
>     strings
>   scsi: mpi3mr: Avoid possible run-time warning with long manufacturer
>     strings
>   scsi: qla2xxx: Avoid possible run-time warning with long model_num
> 
>  drivers/message/fusion/mptsas.c          | 14 +++----
>  drivers/scsi/mpi3mr/mpi3mr_transport.c   | 14 +++----
>  drivers/scsi/mpt3sas/mpt3sas_base.c      |  2 +-
>  drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++----
>  drivers/scsi/qla2xxx/qla_mr.c            |  6 +--
>  include/linux/string.h                   | 49 ++++++++++++++++++++++++
>  lib/strscpy_kunit.c                      | 26 +++++++++++++
>  7 files changed, 93 insertions(+), 32 deletions(-)
> 
> -- 
> 2.34.1
> 

-- 
Kees Cook
Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
Posted by Martin K. Petersen 1 year, 9 months ago
Hi Kees!

>> This series fixes all 4 of the instances I could find in the SCSI
>> subsystem.
>
> Friendly ping. Can the SCSI tree pick this up, or should I take it
> through the hardening tree?

It's on my list of series to review. Have a couple of fires going right
now.

-- 
Martin K. Petersen	Oracle Linux Engineering
Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
Posted by Kees Cook 1 year, 9 months ago
On Wed, Apr 17, 2024 at 08:35:15PM -0400, Martin K. Petersen wrote:
> 
> Hi Kees!
> 
> >> This series fixes all 4 of the instances I could find in the SCSI
> >> subsystem.
> >
> > Friendly ping. Can the SCSI tree pick this up, or should I take it
> > through the hardening tree?
> 
> It's on my list of series to review. Have a couple of fires going right
> now.

Okay, thanks! I just wanted to make sure it got picked up for v6.9 since
it fixes a real-world issue. :)

-- 
Kees Cook