* [PATCH][next] scsi: Replace one-element arrays with flexible-array members @ 2021-11-05 9:11 Gustavo A. R. Silva 2021-11-05 11:24 ` Steffen Maier 0 siblings, 1 reply; 4+ messages in thread From: Gustavo A. R. Silva @ 2021-11-05 9:11 UTC (permalink / raw) To: James E.J. Bottomley, Martin K. Petersen, Hannes Reinecke Cc: linux-scsi, linux-kernel, Gustavo A. R. Silva, linux-hardening, Kees Cook 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(®_pl->port[0].portname, csio_ln_wwpn(ln), 8); + memcpy(®_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, @@ -246,7 +246,7 @@ static inline int fc_ct_ms_fill(struct fc_lport *lport, &entry->type); put_unaligned_be16(len, &entry->len); put_unaligned_be64(lport->wwnn, - (__be64 *)&entry->value[0]); + (__be64 *)&entry->value); /* Manufacturer */ entry = (struct fc_fdmi_attr_entry *)((char *)entry->value + 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 @@ -158,7 +158,7 @@ struct fc_fdmi_port_name { struct fc_fdmi_attr_entry { __be16 type; __be16 len; - __u8 value[1]; + __u8 value[]; } __attribute__((__packed__)); /* @@ -166,7 +166,7 @@ struct fc_fdmi_attr_entry { */ struct fs_fdmi_attrs { __be32 numattrs; - struct fc_fdmi_attr_entry attr[1]; + struct fc_fdmi_attr_entry attr[]; } __attribute__((__packed__)); /* @@ -174,7 +174,7 @@ struct fs_fdmi_attrs { */ struct fc_fdmi_rpl { __be32 numport; - struct fc_fdmi_port_name port[1]; + struct fc_fdmi_port_name port; } __attribute__((__packed__)); /* -- 2.27.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][next] scsi: Replace one-element arrays with flexible-array members 2021-11-05 9:11 [PATCH][next] scsi: Replace one-element arrays with flexible-array members Gustavo A. R. Silva @ 2021-11-05 11:24 ` Steffen Maier 2021-12-02 19:23 ` Kees Cook 0 siblings, 1 reply; 4+ messages in thread From: Steffen Maier @ 2021-11-05 11:24 UTC (permalink / raw) To: Gustavo A. R. Silva, James E.J. Bottomley, Martin K. Petersen, Hannes Reinecke Cc: linux-scsi, linux-kernel, linux-hardening, Kees Cook, Javed Hasan, Himanshu Madhani, Neerav Parikh, Ross Brattain, Robert Love 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(®_pl->port[0].portname, csio_ln_wwpn(ln), 8); > + memcpy(®_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. -- Mit freundlichen Gruessen / Kind regards Steffen Maier Linux on IBM Z and LinuxONE https://www.ibm.com/privacy/us/en/ IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschaeftsfuehrung: Dirk Wittkopp Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][next] scsi: Replace one-element arrays with flexible-array members 2021-11-05 11:24 ` Steffen Maier @ 2021-12-02 19:23 ` Kees Cook 2021-12-02 19:40 ` Gustavo A. R. Silva 0 siblings, 1 reply; 4+ messages in thread From: Kees Cook @ 2021-12-02 19:23 UTC (permalink / raw) To: Steffen Maier Cc: Gustavo A. R. Silva, James E.J. Bottomley, Martin K. Petersen, Hannes Reinecke, linux-scsi, linux-kernel, linux-hardening, Javed Hasan, Himanshu Madhani, Neerav Parikh, Ross Brattain, Robert Love 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(®_pl->port[0].portname, csio_ln_wwpn(ln), 8); > > + memcpy(®_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. 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. For struct fc_dmi_rpl, as long as "numport" is always set/validated to 1, I think this change is fine. -- Kees Cook ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][next] scsi: Replace one-element arrays with flexible-array members 2021-12-02 19:23 ` Kees Cook @ 2021-12-02 19:40 ` Gustavo A. R. Silva 0 siblings, 0 replies; 4+ messages in thread From: Gustavo A. R. Silva @ 2021-12-02 19:40 UTC (permalink / raw) To: Kees Cook Cc: Steffen Maier, James E.J. Bottomley, Martin K. Petersen, Hannes Reinecke, linux-scsi, linux-kernel, linux-hardening, Javed Hasan, Himanshu Madhani, Neerav Parikh, Ross Brattain, Robert Love 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(®_pl->port[0].portname, csio_ln_wwpn(ln), 8); > > > + memcpy(®_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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-02 19:35 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-05 9:11 [PATCH][next] scsi: Replace one-element arrays with flexible-array members Gustavo A. R. Silva 2021-11-05 11:24 ` Steffen Maier 2021-12-02 19:23 ` Kees Cook 2021-12-02 19:40 ` Gustavo A. R. Silva
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.