[PATCH 0/3] scsi: Fix a handful of memcpy() field overflows

[PATCH 0/3] scsi: Fix a handful of memcpy() field overflows

[Kees Cook]

While working on improving FORTIFY_SOURCE's memcpy() coverage, there are
a few fixes that don't require any helper changes, etc.

-Kees

Kees Cook (3):
  scsi: fcoe: Statically initialize flogi_maddr
  scsi: esas2r: Switch to flexible array member
  scsi: isci: Use correctly sized target buffer for memcpy()

 drivers/scsi/esas2r/atioctl.h | 2 +-
 drivers/scsi/fcoe/fcoe.c      | 6 ++----
 drivers/scsi/isci/task.c      | 4 ++--
 3 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/3] scsi: fcoe: Statically initialize flogi_maddr

[Kees Cook]

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy() using memcpy() with an inline const
buffer and instead just statically initialize the destination array
directly.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/fcoe/fcoe.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 89ec735929c3..8991990e6639 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -293,7 +293,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
 	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
 	struct netdev_hw_addr *ha;
 	struct net_device *real_dev;
-	u8 flogi_maddr[ETH_ALEN];
+	u8 flogi_maddr[ETH_ALEN] = FC_FCOE_FLOGI_MAC;
 	const struct net_device_ops *ops;
 
 	fcoe->netdev = netdev;
@@ -336,7 +336,6 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
 	 * or enter promiscuous mode if not capable of listening
 	 * for multiple unicast MACs.
 	 */
-	memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
 	dev_uc_add(netdev, flogi_maddr);
 	if (fip->spma)
 		dev_uc_add(netdev, fip->ctl_src_addr);
@@ -442,7 +441,7 @@ static void fcoe_interface_remove(struct fcoe_interface *fcoe)
 {
 	struct net_device *netdev = fcoe->netdev;
 	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
-	u8 flogi_maddr[ETH_ALEN];
+	u8 flogi_maddr[ETH_ALEN] = FC_FCOE_FLOGI_MAC;
 	const struct net_device_ops *ops;
 
 	/*
@@ -458,7 +457,6 @@ static void fcoe_interface_remove(struct fcoe_interface *fcoe)
 	synchronize_net();
 
 	/* Delete secondary MAC addresses */
-	memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
 	dev_uc_del(netdev, flogi_maddr);
 	if (fip->spma)
 		dev_uc_del(netdev, fip->ctl_src_addr);
-- 
2.25.1

Re: [PATCH 1/3] scsi: fcoe: Statically initialize flogi_maddr

[Joe Perches]

On Fri, 2021-05-28 at 11:13 -0700, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memcpy() using memcpy() with an inline const
> buffer and instead just statically initialize the destination array
> directly.
[]
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
[]
> @@ -293,7 +293,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
>  	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
>  	struct netdev_hw_addr *ha;
>  	struct net_device *real_dev;
> -	u8 flogi_maddr[ETH_ALEN];
> +	u8 flogi_maddr[ETH_ALEN] = FC_FCOE_FLOGI_MAC;

static const

> @@ -442,7 +441,7 @@ static void fcoe_interface_remove(struct fcoe_interface *fcoe)
>  {
>  	struct net_device *netdev = fcoe->netdev;
>  	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
> -	u8 flogi_maddr[ETH_ALEN];
> +	u8 flogi_maddr[ETH_ALEN] = FC_FCOE_FLOGI_MAC;

etc...

Re: [PATCH 1/3] scsi: fcoe: Statically initialize flogi_maddr

[Kees Cook]

On Fri, May 28, 2021 at 11:28:59AM -0700, Joe Perches wrote:
> On Fri, 2021-05-28 at 11:13 -0700, Kees Cook wrote:
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memcpy() using memcpy() with an inline const
> > buffer and instead just statically initialize the destination array
> > directly.
> []
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> []
> > @@ -293,7 +293,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
> >  	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
> >  	struct netdev_hw_addr *ha;
> >  	struct net_device *real_dev;
> > -	u8 flogi_maddr[ETH_ALEN];
> > +	u8 flogi_maddr[ETH_ALEN] = FC_FCOE_FLOGI_MAC;
> 
> static const
> 
> > @@ -442,7 +441,7 @@ static void fcoe_interface_remove(struct fcoe_interface *fcoe)
> >  {
> >  	struct net_device *netdev = fcoe->netdev;
> >  	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
> > -	u8 flogi_maddr[ETH_ALEN];
> > +	u8 flogi_maddr[ETH_ALEN] = FC_FCOE_FLOGI_MAC;
> 
> etc...

Hm, good point.

-- 
Kees Cook

[PATCH 2/3] scsi: esas2r: Switch to flexible array member

[Kees Cook]

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), avoid intentionally writing across
neighboring array fields.

Remove old-style 1-byte array in favor of a flexible array[1] to avoid
future false-positive cross-field memcpy() warning in:

esas2r_vda.c:
	memcpy(vi->cmd.gsv.version_info, esas2r_vdaioctl_versions, ...)

The change in struct size doesn't change other structure sizes (it is
already maxed out to 256 bytes, for example here:

        union {
                struct atto_ioctl_vda_scsi_cmd scsi;
                struct atto_ioctl_vda_flash_cmd flash;
                struct atto_ioctl_vda_diag_cmd diag;
                struct atto_ioctl_vda_cli_cmd cli;
                struct atto_ioctl_vda_smp_cmd smp;
                struct atto_ioctl_vda_cfg_cmd cfg;
                struct atto_ioctl_vda_mgt_cmd mgt;
                struct atto_ioctl_vda_gsv_cmd gsv;
                u8 cmd_info[256];
        } cmd;

No sizes are calculated using the enclosing structure, so no other
updates are needed.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/esas2r/atioctl.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/esas2r/atioctl.h b/drivers/scsi/esas2r/atioctl.h
index 4aca3d52c851..ff2ad9b38575 100644
--- a/drivers/scsi/esas2r/atioctl.h
+++ b/drivers/scsi/esas2r/atioctl.h
@@ -1141,7 +1141,7 @@ struct __packed atto_ioctl_vda_gsv_cmd {
 
 	u8 rsp_len;
 	u8 reserved[7];
-	u8 version_info[1];
+	u8 version_info[];
 	#define ATTO_VDA_VER_UNSUPPORTED 0xFF
 
 };
-- 
2.25.1

Re: [PATCH 2/3] scsi: esas2r: Switch to flexible array member

[Gustavo A. R. Silva]

On 5/28/21 13:13, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memcpy(), avoid intentionally writing across
> neighboring array fields.
> 
> Remove old-style 1-byte array in favor of a flexible array[1] to avoid
> future false-positive cross-field memcpy() warning in:
> 
> esas2r_vda.c:
> 	memcpy(vi->cmd.gsv.version_info, esas2r_vdaioctl_versions, ...)
> 
> The change in struct size doesn't change other structure sizes (it is
> already maxed out to 256 bytes, for example here:
> 
>         union {
>                 struct atto_ioctl_vda_scsi_cmd scsi;
>                 struct atto_ioctl_vda_flash_cmd flash;
>                 struct atto_ioctl_vda_diag_cmd diag;
>                 struct atto_ioctl_vda_cli_cmd cli;
>                 struct atto_ioctl_vda_smp_cmd smp;
>                 struct atto_ioctl_vda_cfg_cmd cfg;
>                 struct atto_ioctl_vda_mgt_cmd mgt;
>                 struct atto_ioctl_vda_gsv_cmd gsv;
>                 u8 cmd_info[256];
>         } cmd;
> 
> No sizes are calculated using the enclosing structure, so no other
> updates are needed.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
--
Gustavo

> ---
>  drivers/scsi/esas2r/atioctl.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/esas2r/atioctl.h b/drivers/scsi/esas2r/atioctl.h
> index 4aca3d52c851..ff2ad9b38575 100644
> --- a/drivers/scsi/esas2r/atioctl.h
> +++ b/drivers/scsi/esas2r/atioctl.h
> @@ -1141,7 +1141,7 @@ struct __packed atto_ioctl_vda_gsv_cmd {
>  
>  	u8 rsp_len;
>  	u8 reserved[7];
> -	u8 version_info[1];
> +	u8 version_info[];
>  	#define ATTO_VDA_VER_UNSUPPORTED 0xFF
>  
>  };
> 

[PATCH 3/3] scsi: isci: Use correctly sized target buffer for memcpy()

[Kees Cook]

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), avoid intentionally writing across
neighboring array fields.

Switch from rsp_ui to resp_buf, since resp_ui isn't SSP_RESP_IU_MAX_SIZE
bytes in length. This avoids future compile-time warnings.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/isci/task.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/isci/task.c b/drivers/scsi/isci/task.c
index 62062ed6cd9a..eeaec26ac324 100644
--- a/drivers/scsi/isci/task.c
+++ b/drivers/scsi/isci/task.c
@@ -709,8 +709,8 @@ isci_task_request_complete(struct isci_host *ihost,
 		tmf->status = completion_status;
 
 		if (tmf->proto == SAS_PROTOCOL_SSP) {
-			memcpy(&tmf->resp.resp_iu,
-			       &ireq->ssp.rsp,
+			memcpy(tmf->resp.rsp_buf,
+			       ireq->ssp.rsp_buf,
 			       SSP_RESP_IU_MAX_SIZE);
 		} else if (tmf->proto == SAS_PROTOCOL_SATA) {
 			memcpy(&tmf->resp.d2h_fis,
-- 
2.25.1

Re: [PATCH 3/3] scsi: isci: Use correctly sized target buffer for memcpy()

[Gustavo A. R. Silva]

On 5/28/21 13:13, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memcpy(), avoid intentionally writing across
> neighboring array fields.
> 
> Switch from rsp_ui to resp_buf, since resp_ui isn't SSP_RESP_IU_MAX_SIZE
> bytes in length. This avoids future compile-time warnings.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
--
Gustavo

> ---
>  drivers/scsi/isci/task.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/isci/task.c b/drivers/scsi/isci/task.c
> index 62062ed6cd9a..eeaec26ac324 100644
> --- a/drivers/scsi/isci/task.c
> +++ b/drivers/scsi/isci/task.c
> @@ -709,8 +709,8 @@ isci_task_request_complete(struct isci_host *ihost,
>  		tmf->status = completion_status;
>  
>  		if (tmf->proto == SAS_PROTOCOL_SSP) {
> -			memcpy(&tmf->resp.resp_iu,
> -			       &ireq->ssp.rsp,
> +			memcpy(tmf->resp.rsp_buf,
> +			       ireq->ssp.rsp_buf,
>  			       SSP_RESP_IU_MAX_SIZE);
>  		} else if (tmf->proto == SAS_PROTOCOL_SATA) {
>  			memcpy(&tmf->resp.d2h_fis,
> 

Re: [PATCH 0/3] scsi: Fix a handful of memcpy() field overflows

[Martin K. Petersen]

Kees,

> While working on improving FORTIFY_SOURCE's memcpy() coverage, there are
> a few fixes that don't require any helper changes, etc.

Applied patches 2 and 3 to 5.14/scsi-staging, please update patch 1.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/3] scsi: Fix a handful of memcpy() field overflows

[Kees Cook]

On Tue, Jun 01, 2021 at 11:38:47PM -0400, Martin K. Petersen wrote:
> Kees,
> 
> > While working on improving FORTIFY_SOURCE's memcpy() coverage, there are
> > a few fixes that don't require any helper changes, etc.
> 
> Applied patches 2 and 3 to 5.14/scsi-staging, please update patch 1.
> 
> Thanks!

Awesome; thanks! I've sent v2 for patch 1 now.

-- 
Kees Cook

Re: [PATCH 0/3] scsi: Fix a handful of memcpy() field overflows

[Martin K. Petersen]

On Fri, 28 May 2021 11:13:34 -0700, Kees Cook wrote:

> While working on improving FORTIFY_SOURCE's memcpy() coverage, there are
> a few fixes that don't require any helper changes, etc.
> 
> -Kees
> 
> Kees Cook (3):
>   scsi: fcoe: Statically initialize flogi_maddr
>   scsi: esas2r: Switch to flexible array member
>   scsi: isci: Use correctly sized target buffer for memcpy()
> 
> [...]

Applied to 5.14/scsi-queue, thanks!

[2/3] scsi: esas2r: Switch to flexible array member
      https://git.kernel.org/mkp/scsi/c/66fc475bd9e1
[3/3] scsi: isci: Use correctly sized target buffer for memcpy()
      https://git.kernel.org/mkp/scsi/c/5250db63d140

-- 
Martin K. Petersen	Oracle Linux Engineering