2023-10-23 16:31:28

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups

On Sun, Aug 06, 2023 at 10:05:52AM -0700, James Seo wrote:
> Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
> resulted in the only arrays that UBSAN_BOUNDS considers unbounded
> being trailing arrays declared with [] as the last member of a
> struct. Unbounded trailing arrays declared with [1] are common in
> mpt3sas, which is causing spurious warnings to appear in some
> situations, e.g. when more than one physical disk is connected:
>
> UBSAN: array-index-out-of-bounds in drivers/scsi/mpt3sas/mpt3sas_scsih.c:6810:36
> index 1 is out of range for type 'MPI2_SAS_IO_UNIT0_PHY_DATA [1]'
>
> which relates to this unbounded array access:
>
> port_id = sas_iounit_pg0->PhyData[i].Port;
>
> and is just one example of 10 similar warnings currently occurring
> for me during boot.
>
> This series converts most trailing arrays declared with [1] in mptsas
> into proper C99 flexible array members. Those that are not unbounded
> and really are fixed-length arrays of length 1 are left alone.
>
> I didn't find any conversions that required further source edits
> besides changing [1] to [], and everything seems to work with my
> SAS2008-based add-in card, but please look things over in case I
> missed something subtle.
>
> Rounding out the series are some opportunistic cleanups.
>
> The only dependency is that patch 7 ("Use struct_size() for struct
> size calculations") depends on patches 3-5.
>
> History:
> v1: https://lore.kernel.org/linux-scsi/[email protected]/
>
> Changes v1->v2:
> - Slightly reword and add Reviewed-by: tags to commit messages
> - Split up a commit that was resulting in many binary changes
> - Remove the iounit_pg8 member of the per-adapter struct
> - Replace more dynamic allocations with local variables

Here's a tested-by: from Boris:

https://lore.kernel.org/all/20231023135615.GBZTZ7fwRh48euq3ew@fat_crate.local

-Kees

>
> James Seo (12):
> scsi: mpt3sas: Use flexible arrays when obviously possible
> scsi: mpt3sas: Make MPI2_CONFIG_PAGE_IO_UNIT_8::Sensor[] a flexible
> array
> scsi: mpt3sas: Make MPI2_CONFIG_PAGE_RAID_VOL_0::PhysDisk[] a flexible
> array
> scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_0::PhyData[] a flexible
> array
> scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_1::PhyData[] a flexible
> array
> scsi: mpt3sas: Make MPI26_CONFIG_PAGE_PIOUNIT_1::PhyData[] a flexible
> array
> scsi: mpt3sas: Use struct_size() for struct size calculations
> scsi: mpt3sas: Remove the iounit_pg8 member of the per-adapter struct
> scsi: mpt3sas: Fix an outdated comment
> scsi: mpt3sas: Fix typo of "TRIGGER"
> scsi: mpt3sas: Replace a dynamic allocation with a local variable
> scsi: mpt3sas: Replace dynamic allocations with local variables
>
> drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 231 ++++++-------------
> drivers/scsi/mpt3sas/mpi/mpi2_image.h | 32 +--
> drivers/scsi/mpt3sas/mpi/mpi2_ioc.h | 27 +--
> drivers/scsi/mpt3sas/mpt3sas_base.c | 35 ++-
> drivers/scsi/mpt3sas/mpt3sas_base.h | 2 -
> drivers/scsi/mpt3sas/mpt3sas_config.c | 6 +-
> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 55 ++---
> drivers/scsi/mpt3sas/mpt3sas_transport.c | 9 +-
> drivers/scsi/mpt3sas/mpt3sas_trigger_pages.h | 44 ++--
> drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 3 +-
> 10 files changed, 151 insertions(+), 293 deletions(-)
>
>
> base-commit: 6cae9a3910ac1b5daf5ac3db9576b78cc4eff5aa
> --
> 2.39.2
>

--
Kees Cook


2023-10-25 02:06:24

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups


Kees,

> Here's a tested-by: from Boris:
>
> https://lore.kernel.org/all/20231023135615.GBZTZ7fwRh48euq3ew@fat_crate.local

I'm a bit concerned bringing this in just before the merge window.
Please ping me if I forget to merge once -rc1 is out.

--
Martin K. Petersen Oracle Linux Engineering

2023-10-25 22:34:07

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups

On Tue, Oct 24, 2023 at 10:05:33PM -0400, Martin K. Petersen wrote:
>
> Kees,
>
> > Here's a tested-by: from Boris:
> >
> > https://lore.kernel.org/all/20231023135615.GBZTZ7fwRh48euq3ew@fat_crate.local
>
> I'm a bit concerned bringing this in just before the merge window.
> Please ping me if I forget to merge once -rc1 is out.

Sounds good; thanks!

--
Kees Cook

2023-11-15 13:55:12

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups


Kees,

>> I'm a bit concerned bringing this in just before the merge window.
>> Please ping me if I forget to merge once -rc1 is out.

Applied to 6.8/scsi-staging, thanks!

--
Martin K. Petersen Oracle Linux Engineering

2023-11-15 14:39:04

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups

On Wed, Nov 15, 2023 at 08:54:22AM -0500, Martin K. Petersen wrote:
> >> I'm a bit concerned bringing this in just before the merge window.
> >> Please ping me if I forget to merge once -rc1 is out.
>
> Applied to 6.8/scsi-staging, thanks!

Great! Thanks for picking this up. :)

--
Kees Cook