Date:   Thu, 2 Dec 2021 13:40:20 -0600
From:   "Gustavo A. R. Silva" <gustavoars@kernel.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     Steffen Maier <maier@linux.ibm.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Hannes Reinecke <hare@suse.de>, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
        Javed Hasan <jhasan@marvell.com>,
        Himanshu Madhani <himanshu.madhani@oracle.com>,
        Neerav Parikh <neerav.parikh@intel.com>,
        Ross Brattain <ross.b.brattain@intel.com>,
        Robert Love <robert.w.love@intel.com>
Subject: Re: [PATCH][next] scsi: Replace one-element arrays with
 flexible-array members
Message-ID: <20211202194020.GA13288@embeddedor>
References: <20211105091102.GA126301@embeddedor>
 <22139a80-3f64-1f21-6b5c-65d250bafe09@linux.ibm.com>
 <202112021113.D71F11B70@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202112021113.D71F11B70@keescook>
Precedence: bulk

On Thu, Dec 02, 2021 at 11:23:26AM -0800, Kees Cook wrote:
> On Fri, Nov 05, 2021 at 12:24:13PM +0100, Steffen Maier wrote:
> > On 11/5/21 10:11, Gustavo A. R. Silva wrote:
> > > Use flexible-array members in struct fc_fdmi_attr_entry and
> > > fs_fdmi_attrs instead of one-element arrays, and refactor the
> > > code accordingly.
> > > 
> > > Also, turn the one-element array _port_ in struct fc_fdmi_rpl
> > > into a simple object of type struct fc_fdmi_port_name, as it
> > > seems there is no more than just one port expected:
> > > 
> > > $ git grep -nw numport drivers/scsi/
> > > drivers/scsi/csiostor/csio_lnode.c:447: reg_pl->numport = htonl(1);
> > > drivers/scsi/libfc/fc_encode.h:232:             put_unaligned_be32(1, &ct->payload.rhba.port.numport);
> > > 
> > > Also, this helps with the ongoing efforts to globally enable
> > > -Warray-bounds and get us closer to being able to tighten the
> > > FORTIFY_SOURCE routines on memcpy().
> > > 
> > > https://github.com/KSPP/linux/issues/79
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > ---
> > >   drivers/scsi/csiostor/csio_lnode.c | 2 +-
> > >   drivers/scsi/libfc/fc_encode.h     | 4 ++--
> > >   include/scsi/fc/fc_ms.h            | 6 +++---
> > >   3 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/csiostor/csio_lnode.c b/drivers/scsi/csiostor/csio_lnode.c
> > > index d5ac93897023..cf9dd79ee488 100644
> > > --- a/drivers/scsi/csiostor/csio_lnode.c
> > > +++ b/drivers/scsi/csiostor/csio_lnode.c
> > > @@ -445,7 +445,7 @@ csio_ln_fdmi_dprt_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req)
> > >   	/* Register one port per hba */
> > >   	reg_pl = (struct fc_fdmi_rpl *)pld;
> > >   	reg_pl->numport = htonl(1);
> > > -	memcpy(&reg_pl->port[0].portname, csio_ln_wwpn(ln), 8);
> > > +	memcpy(&reg_pl->port.portname, csio_ln_wwpn(ln), 8);
> > >   	pld += sizeof(*reg_pl);
> > > 
> > >   	/* Start appending HBA attributes hba */
> > > diff --git a/drivers/scsi/libfc/fc_encode.h b/drivers/scsi/libfc/fc_encode.h
> > > index 74ae7fd15d8d..5806f99e4061 100644
> > > --- a/drivers/scsi/libfc/fc_encode.h
> > > +++ b/drivers/scsi/libfc/fc_encode.h
> > > @@ -232,7 +232,7 @@ static inline int fc_ct_ms_fill(struct fc_lport *lport,
> > >   		put_unaligned_be32(1, &ct->payload.rhba.port.numport);
> > >   		/* Port Name */
> > >   		put_unaligned_be64(lport->wwpn,
> > > -				   &ct->payload.rhba.port.port[0].portname);
> > > +				   &ct->payload.rhba.port.port.portname);
> > > 
> > >   		/* HBA Attributes */
> > >   		put_unaligned_be32(numattrs,
> > 
> > > diff --git a/include/scsi/fc/fc_ms.h b/include/scsi/fc/fc_ms.h
> > > index 00191695233a..44fbe84fa664 100644
> > > --- a/include/scsi/fc/fc_ms.h
> > > +++ b/include/scsi/fc/fc_ms.h
> > 
> > > @@ -174,7 +174,7 @@ struct fs_fdmi_attrs {
> > 
> > /*
> >  * Registered Port List
> > 
> > >    */
> > >   struct fc_fdmi_rpl {
> > >   	__be32				numport;
> > > -	struct fc_fdmi_port_name	port[1];
> > > +	struct fc_fdmi_port_name	port;
> > >   } __attribute__((__packed__));
> > 
> > While I'm not affected by the change, it feels to me as if these are
> > protocol definitions originating in a T11 Fibre Channel standard FC-GS. It's
> > a port *list*. Can you "modify" the standard here?
> > 
> > The fact, that currently existing code users only ever seem to use one
> > single port in the list, would be an independent thing to me.
> 
> There are three changes made here, and I suspect it might make sense to
> split them up.
> 
> In a quick look, I see "struct fc_fdmi_attr_entry" has a sizeof() call
> against it, so it's not clear if it's safe to switch it to a flexible
> array without other changes.

I'll take a look.

> The change to struct fs_fdmi_attrs looks okay, since it appears to be
> used only in casts, but it might make sense to use diffoscope on the
> changed .o files to validate nothing weird has happened.

OK. 

> For struct fc_dmi_rpl, as long as "numport" is always set/validated to
> 1, I think this change is fine.

OK. I'll split this up into a small series and keep

	struct fc_fdmi_port_name	port[1];

as a one-element array.

Thanks
--
Gustavo