[PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members

[PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members

[Gustavo A. R. Silva]

This series aims to replace one-element arrays with flexible-array
members in multiple structures in drivers/scsi/aacraid/aacraid.h.

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().

These issues were found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79

Gustavo A. R. Silva (10):
  scsi: aacraid: Replace one-element array with flexible-array member
  scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns()
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct aac_aifcmd
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct user_sgmapraw
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct sgmapraw
  scsi: aacraid: Use struct_size() helper in code related to struct
    sgmapraw
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct user_sgmap64
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct sgmap
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct sgmap64
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct user_sgmap

 drivers/scsi/aacraid/aachba.c   | 42 +++++++++++++--------------------
 drivers/scsi/aacraid/aacraid.h  | 16 ++++++-------
 drivers/scsi/aacraid/commctrl.c |  6 ++---
 drivers/scsi/aacraid/comminit.c |  3 +--
 4 files changed, 28 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
aac_ciss_phys_luns_resp.

This results in no differences in binary output.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aachba.c  | 2 +-
 drivers/scsi/aacraid/aacraid.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 70e1cac1975e..bba2d4d952ca 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1833,7 +1833,7 @@ static int aac_get_safw_ciss_luns(struct aac_dev *dev)
 	struct aac_ciss_phys_luns_resp *phys_luns;
 
 	datasize = sizeof(struct aac_ciss_phys_luns_resp) +
-		(AAC_MAX_TARGETS - 1) * sizeof(struct _ciss_lun);
+		   AAC_MAX_TARGETS * sizeof(struct _ciss_lun);
 	phys_luns = kmalloc(datasize, GFP_KERNEL);
 	if (phys_luns == NULL)
 		goto out;
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 7c6efde75da6..83fa890f28be 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -322,7 +322,7 @@ struct aac_ciss_phys_luns_resp {
 		u8	level3[2];
 		u8	level2[2];
 		u8	node_ident[16];	/* phys. node identifier */
-	} lun[1];			/* List of phys. devices */
+	} lun[];			/* List of phys. devices */
 };
 
 /*
-- 
2.34.1

Re: [PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member

[Kees Cook]

On Wed, Jun 28, 2023 at 11:54:27AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> aac_ciss_phys_luns_resp.

I think the Subject was cut off (it lacks mention of
aac_ciss_phys_luns_resp, where as the other patches mention the target
struct).

> This results in no differences in binary output.

Confirmed.

> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 02/10][next] scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns()

[Gustavo A. R. Silva]

Prefer struct_size() over open-coded versions.

This results in no differences in binary output.

Link: https://github.com/KSPP/linux/issues/160
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aachba.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index bba2d4d952ca..fff0550e02e4 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1832,8 +1832,7 @@ static int aac_get_safw_ciss_luns(struct aac_dev *dev)
 	struct aac_srb_unit srbu;
 	struct aac_ciss_phys_luns_resp *phys_luns;
 
-	datasize = sizeof(struct aac_ciss_phys_luns_resp) +
-		   AAC_MAX_TARGETS * sizeof(struct _ciss_lun);
+	datasize = struct_size(phys_luns, lun, AAC_MAX_TARGETS);
 	phys_luns = kmalloc(datasize, GFP_KERNEL);
 	if (phys_luns == NULL)
 		goto out;
-- 
2.34.1

Re: [PATCH 02/10][next] scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns()

[Kees Cook]

On Wed, Jun 28, 2023 at 11:54:58AM -0600, Gustavo A. R. Silva wrote:
> Prefer struct_size() over open-coded versions.
> 
> This results in no differences in binary output.
> 
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
aac_aifcmd.

This results in no differences in binary output.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aacraid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 83fa890f28be..2e1623344327 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -2618,7 +2618,7 @@ struct aac_hba_info {
 struct aac_aifcmd {
 	__le32 command;		/* Tell host what type of notify this is */
 	__le32 seqnum;		/* To allow ordering of reports (if necessary) */
-	u8 data[1];		/* Undefined length (from kernel viewpoint) */
+	u8 data[];		/* Undefined length (from kernel viewpoint) */
 };
 
 /**
-- 
2.34.1

Re: [PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd

[Kees Cook]

On Wed, Jun 28, 2023 at 11:55:24AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> aac_aifcmd.
> 
> This results in no differences in binary output.

Confirmed.

> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
user_sgmapraw.

This results in no differences in binary output.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aacraid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 2e1623344327..d1fc1ce2e36d 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -532,7 +532,7 @@ struct sgmapraw {
 
 struct user_sgmapraw {
 	u32		  count;
-	struct user_sgentryraw sg[1];
+	struct user_sgentryraw sg[];
 };
 
 struct creation_info
-- 
2.34.1

Re: [PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw

[Kees Cook]

On Wed, Jun 28, 2023 at 11:55:47AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> user_sgmapraw.
> 
> This results in no differences in binary output.

Confirmed.

> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
sgmapraw.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aachba.c  | 4 ++--
 drivers/scsi/aacraid/aacraid.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index fff0550e02e4..b3c0c2255e55 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1267,7 +1267,7 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
 			return ret;
 		command = ContainerRawIo;
 		fibsize = sizeof(struct aac_raw_io) +
-			((le32_to_cpu(readcmd->sg.count)-1) * sizeof(struct sgentryraw));
+			  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentryraw);
 	}
 
 	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));
@@ -1401,7 +1401,7 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
 			return ret;
 		command = ContainerRawIo;
 		fibsize = sizeof(struct aac_raw_io) +
-			((le32_to_cpu(writecmd->sg.count)-1) * sizeof (struct sgentryraw));
+			  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentryraw);
 	}
 
 	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index d1fc1ce2e36d..87015dd2abd9 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -527,7 +527,7 @@ struct user_sgmap64 {
 
 struct sgmapraw {
 	__le32		  count;
-	struct sgentryraw sg[1];
+	struct sgentryraw sg[];
 };
 
 struct user_sgmapraw {
-- 
2.34.1

Re: [PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw

[Kees Cook]

On Wed, Jun 28, 2023 at 11:56:12AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> sgmapraw.
> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.

As with the other two, I see expected binary changes in
aac_read_raw_io() and aac_write_raw_io() due to the simplified count
calculations.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/scsi/aacraid/aachba.c  | 4 ++--
>  drivers/scsi/aacraid/aacraid.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index fff0550e02e4..b3c0c2255e55 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1267,7 +1267,7 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
>  			return ret;
>  		command = ContainerRawIo;
>  		fibsize = sizeof(struct aac_raw_io) +
> -			((le32_to_cpu(readcmd->sg.count)-1) * sizeof(struct sgentryraw));
> +			  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentryraw);
>  	}
>  
>  	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));
> @@ -1401,7 +1401,7 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
>  			return ret;
>  		command = ContainerRawIo;
>  		fibsize = sizeof(struct aac_raw_io) +
> -			((le32_to_cpu(writecmd->sg.count)-1) * sizeof (struct sgentryraw));
> +			  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentryraw);
>  	}
>  
>  	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index d1fc1ce2e36d..87015dd2abd9 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -527,7 +527,7 @@ struct user_sgmap64 {
>  
>  struct sgmapraw {
>  	__le32		  count;
> -	struct sgentryraw sg[1];
> +	struct sgentryraw sg[];
>  };
>  
>  struct user_sgmapraw {
> -- 
> 2.34.1
> 

-- 
Kees Cook

[PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to struct sgmapraw

[Gustavo A. R. Silva]

Prefer struct_size() over open-coded versions.

Link: https://github.com/KSPP/linux/issues/160
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aachba.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index b3c0c2255e55..03ba974f6b2a 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1266,8 +1266,7 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
 		if (ret < 0)
 			return ret;
 		command = ContainerRawIo;
-		fibsize = sizeof(struct aac_raw_io) +
-			  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentryraw);
+		fibsize = struct_size(readcmd, sg.sg, le32_to_cpu(readcmd->sg.count));
 	}
 
 	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));
@@ -1400,8 +1399,7 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
 		if (ret < 0)
 			return ret;
 		command = ContainerRawIo;
-		fibsize = sizeof(struct aac_raw_io) +
-			  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentryraw);
+		fibsize = struct_size(writecmd, sg.sg, le32_to_cpu(writecmd->sg.count));
 	}
 
 	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));
-- 
2.34.1

Re: [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to struct sgmapraw

[Kees Cook]

On Wed, Jun 28, 2023 at 11:56:31AM -0600, Gustavo A. R. Silva wrote:
> Prefer struct_size() over open-coded versions.
> 
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to struct sgmapraw

[Kees Cook]

On Wed, Jun 28, 2023 at 11:56:31AM -0600, Gustavo A. R. Silva wrote:
> Prefer struct_size() over open-coded versions.

Oh, I think you can add two more patches to convert sgmap and sgmap64
fibsize calculations too.

-- 
Kees Cook

[PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
user_sgmap64.

This results in no differences in binary output.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aacraid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 87015dd2abd9..94eb83d38be6 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -522,7 +522,7 @@ struct sgmap64 {
 
 struct user_sgmap64 {
 	u32		count;
-	struct user_sgentry64 sg[1];
+	struct user_sgentry64 sg[];
 };
 
 struct sgmapraw {
-- 
2.34.1

Re: [PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64

[Kees Cook]

On Wed, Jun 28, 2023 at 11:56:52AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> user_sgmap64.
> 
> This results in no differences in binary output.

Confirmed.

> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
sgmap and refactor the rest of the code, accordingly.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aachba.c   | 24 ++++++++++--------------
 drivers/scsi/aacraid/aacraid.h  |  2 +-
 drivers/scsi/aacraid/commctrl.c |  4 ++--
 drivers/scsi/aacraid/comminit.c |  3 +--
 4 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 03ba974f6b2a..b2849e5cc104 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1336,8 +1336,7 @@ static int aac_read_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u32
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_read) +
-			((le32_to_cpu(readcmd->sg.count) - 1) *
-			 sizeof (struct sgentry));
+		  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentry);
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 	/*
@@ -1471,8 +1470,7 @@ static int aac_write_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_write) +
-		((le32_to_cpu(writecmd->sg.count) - 1) *
-		 sizeof (struct sgentry));
+		  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentry);
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 	/*
@@ -1590,9 +1588,9 @@ static int aac_scsi_64(struct fib * fib, struct scsi_cmnd * cmd)
 	/*
 	 *	Build Scatter/Gather list
 	 */
-	fibsize = sizeof (struct aac_srb) - sizeof (struct sgentry) +
-		((le32_to_cpu(srbcmd->sg.count) & 0xff) *
-		 sizeof (struct sgentry64));
+	fibsize = sizeof(struct aac_srb) +
+		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
+		  sizeof(struct sgentry64);
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 
@@ -1621,9 +1619,9 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
 	/*
 	 *	Build Scatter/Gather list
 	 */
-	fibsize = sizeof (struct aac_srb) +
-		(((le32_to_cpu(srbcmd->sg.count) & 0xff) - 1) *
-		 sizeof (struct sgentry));
+	fibsize = sizeof(struct aac_srb) +
+		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
+		  sizeof(struct sgentry);
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 
@@ -1691,8 +1689,7 @@ static int aac_send_safw_bmic_cmd(struct aac_dev *dev,
 	fibptr->hw_fib_va->header.XferState &=
 		~cpu_to_le32(FastResponseCapable);
 
-	fibsize  = sizeof(struct aac_srb) - sizeof(struct sgentry) +
-						sizeof(struct sgentry64);
+	fibsize  = sizeof(struct aac_srb) + sizeof(struct sgentry64);
 
 	/* allocate DMA buffer for response */
 	addr = dma_map_single(&dev->pdev->dev, xfer_buf, xfer_len,
@@ -2264,8 +2261,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 		dev->a_ops.adapter_bounds = aac_bounds_32;
 		dev->scsi_host_ptr->sg_tablesize = (dev->max_fib_size -
 			sizeof(struct aac_fibhdr) -
-			sizeof(struct aac_write) + sizeof(struct sgentry)) /
-				sizeof(struct sgentry);
+			sizeof(struct aac_write)) / sizeof(struct sgentry);
 		if (dev->dac_support) {
 			dev->a_ops.adapter_read = aac_read_block64;
 			dev->a_ops.adapter_write = aac_write_block64;
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 94eb83d38be6..3fbc22ae72b6 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -507,7 +507,7 @@ struct sge_ieee1212 {
 
 struct sgmap {
 	__le32		count;
-	struct sgentry	sg[1];
+	struct sgentry	sg[];
 };
 
 struct user_sgmap {
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index e7cc927ed952..df811ad4afaa 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -561,8 +561,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		rcode = -EINVAL;
 		goto cleanup;
 	}
-	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
-		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
+	actual_fibsize = sizeof(struct aac_srb) +
+		(user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry);
 	actual_fibsize64 = actual_fibsize + (user_srbcmd->sg.count & 0xff) *
 	  (sizeof(struct sgentry64) - sizeof(struct sgentry));
 	/* User made a mistake - should not continue */
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index bd99c5492b7d..d8dd89c87b01 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -523,8 +523,7 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
 	dev->max_fib_size = sizeof(struct hw_fib);
 	dev->sg_tablesize = host->sg_tablesize = (dev->max_fib_size
 		- sizeof(struct aac_fibhdr)
-		- sizeof(struct aac_write) + sizeof(struct sgentry))
-			/ sizeof(struct sgentry);
+		- sizeof(struct aac_write)) / sizeof(struct sgentry);
 	dev->comm_interface = AAC_COMM_PRODUCER;
 	dev->raw_io_interface = dev->raw_io_64 = 0;
 
-- 
2.34.1

Re: [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap

[Kees Cook]

On Wed, Jun 28, 2023 at 11:57:13AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> sgmap and refactor the rest of the code, accordingly.
> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.

This change _does_ have binary output differences, although it looks
like you got most of them. I still see:

commsup.o:
-       mov    $0x40,%edx
+       mov    $0x38,%edx

This appears to be the sizeof() here:

        ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
                                FsaNormal, 1, 1, NULL, NULL);

struct aac_srb includes struct sgmap. I think this needs to explicitly
include the 1 sgmap, which seems to be sent in the fibptr:

        srbcmd = (struct aac_srb *)fib_data(fibptr);
	...
        sg64 = (struct sgmap64 *)&srbcmd->sg;
        sg64->count = cpu_to_le32(1);

i.e. "sending 1". This seems to fix it:

-       ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
+       ret = aac_fib_send(ScsiPortCommand64, fibptr,
+                               struct_size(srbcmd, sg.sg, 1),

Then I see changes in both aac_write_block() and aac_scsi_32(), but they
match the changes you made to get the correct size (it's just an easier
calculation for the compiler to perform, so the code is slightly
simplified).

So I think with the hunk I suggested at the start, and a comment on the
(expected) binary changes, this should be good to go.

-Kees

> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/scsi/aacraid/aachba.c   | 24 ++++++++++--------------
>  drivers/scsi/aacraid/aacraid.h  |  2 +-
>  drivers/scsi/aacraid/commctrl.c |  4 ++--
>  drivers/scsi/aacraid/comminit.c |  3 +--
>  4 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index 03ba974f6b2a..b2849e5cc104 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1336,8 +1336,7 @@ static int aac_read_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u32
>  	if (ret < 0)
>  		return ret;
>  	fibsize = sizeof(struct aac_read) +
> -			((le32_to_cpu(readcmd->sg.count) - 1) *
> -			 sizeof (struct sgentry));
> +		  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentry);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  	/*
> @@ -1471,8 +1470,7 @@ static int aac_write_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
>  	if (ret < 0)
>  		return ret;
>  	fibsize = sizeof(struct aac_write) +
> -		((le32_to_cpu(writecmd->sg.count) - 1) *
> -		 sizeof (struct sgentry));
> +		  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentry);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  	/*
> @@ -1590,9 +1588,9 @@ static int aac_scsi_64(struct fib * fib, struct scsi_cmnd * cmd)
>  	/*
>  	 *	Build Scatter/Gather list
>  	 */
> -	fibsize = sizeof (struct aac_srb) - sizeof (struct sgentry) +
> -		((le32_to_cpu(srbcmd->sg.count) & 0xff) *
> -		 sizeof (struct sgentry64));
> +	fibsize = sizeof(struct aac_srb) +
> +		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
> +		  sizeof(struct sgentry64);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  
> @@ -1621,9 +1619,9 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
>  	/*
>  	 *	Build Scatter/Gather list
>  	 */
> -	fibsize = sizeof (struct aac_srb) +
> -		(((le32_to_cpu(srbcmd->sg.count) & 0xff) - 1) *
> -		 sizeof (struct sgentry));
> +	fibsize = sizeof(struct aac_srb) +
> +		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
> +		  sizeof(struct sgentry);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  
> @@ -1691,8 +1689,7 @@ static int aac_send_safw_bmic_cmd(struct aac_dev *dev,
>  	fibptr->hw_fib_va->header.XferState &=
>  		~cpu_to_le32(FastResponseCapable);
>  
> -	fibsize  = sizeof(struct aac_srb) - sizeof(struct sgentry) +
> -						sizeof(struct sgentry64);
> +	fibsize  = sizeof(struct aac_srb) + sizeof(struct sgentry64);
>  
>  	/* allocate DMA buffer for response */
>  	addr = dma_map_single(&dev->pdev->dev, xfer_buf, xfer_len,
> @@ -2264,8 +2261,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
>  		dev->a_ops.adapter_bounds = aac_bounds_32;
>  		dev->scsi_host_ptr->sg_tablesize = (dev->max_fib_size -
>  			sizeof(struct aac_fibhdr) -
> -			sizeof(struct aac_write) + sizeof(struct sgentry)) /
> -				sizeof(struct sgentry);
> +			sizeof(struct aac_write)) / sizeof(struct sgentry);
>  		if (dev->dac_support) {
>  			dev->a_ops.adapter_read = aac_read_block64;
>  			dev->a_ops.adapter_write = aac_write_block64;
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 94eb83d38be6..3fbc22ae72b6 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -507,7 +507,7 @@ struct sge_ieee1212 {
>  
>  struct sgmap {
>  	__le32		count;
> -	struct sgentry	sg[1];
> +	struct sgentry	sg[];
>  };
>  
>  struct user_sgmap {
> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
> index e7cc927ed952..df811ad4afaa 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -561,8 +561,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>  		rcode = -EINVAL;
>  		goto cleanup;
>  	}
> -	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
> -		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
> +	actual_fibsize = sizeof(struct aac_srb) +
> +		(user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry);
>  	actual_fibsize64 = actual_fibsize + (user_srbcmd->sg.count & 0xff) *
>  	  (sizeof(struct sgentry64) - sizeof(struct sgentry));
>  	/* User made a mistake - should not continue */
> diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
> index bd99c5492b7d..d8dd89c87b01 100644
> --- a/drivers/scsi/aacraid/comminit.c
> +++ b/drivers/scsi/aacraid/comminit.c
> @@ -523,8 +523,7 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
>  	dev->max_fib_size = sizeof(struct hw_fib);
>  	dev->sg_tablesize = host->sg_tablesize = (dev->max_fib_size
>  		- sizeof(struct aac_fibhdr)
> -		- sizeof(struct aac_write) + sizeof(struct sgentry))
> -			/ sizeof(struct sgentry);
> +		- sizeof(struct aac_write)) / sizeof(struct sgentry);
>  	dev->comm_interface = AAC_COMM_PRODUCER;
>  	dev->raw_io_interface = dev->raw_io_64 = 0;
>  
> -- 
> 2.34.1
> 

-- 
Kees Cook

Re: [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap

[Gustavo A. R. Silva]

On 6/28/23 14:36, Kees Cook wrote:
> On Wed, Jun 28, 2023 at 11:57:13AM -0600, Gustavo A. R. Silva wrote:
>> Replace one-element array with flexible-array member in struct
>> sgmap and refactor the rest of the code, accordingly.
>>
>> Issue found with the help of Coccinelle and audited and fixed,
>> manually.
> 
> This change _does_ have binary output differences, although it looks
> like you got most of them. I still see:
> 
> commsup.o:
> -       mov    $0x40,%edx
> +       mov    $0x38,%edx
> 
> This appears to be the sizeof() here:
> 
>          ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
>                                  FsaNormal, 1, 1, NULL, NULL);
> 
> struct aac_srb includes struct sgmap. I think this needs to explicitly
> include the 1 sgmap, which seems to be sent in the fibptr:

I see your point. Yeah; this is one of those cases of nested struct-with-flex-array.

The flex-array is a couple of layers into the main enclosing structure.

However, I would like to have the input of a maintainer here, just to confirm
this would be the expected change.

Thanks a lot for the feedback! :)
--
Gustavo

> 
>          srbcmd = (struct aac_srb *)fib_data(fibptr);
> 	...
>          sg64 = (struct sgmap64 *)&srbcmd->sg;
>          sg64->count = cpu_to_le32(1);
> 
> i.e. "sending 1". This seems to fix it:
> 
> -       ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
> +       ret = aac_fib_send(ScsiPortCommand64, fibptr,
> +                               struct_size(srbcmd, sg.sg, 1),
> 
> Then I see changes in both aac_write_block() and aac_scsi_32(), but they
> match the changes you made to get the correct size (it's just an easier
> calculation for the compiler to perform, so the code is slightly
> simplified).
> 
> So I think with the hunk I suggested at the start, and a comment on the
> (expected) binary changes, this should be good to go.
> 
> -Kees
> 
>> Link: https://github.com/KSPP/linux/issues/79
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>   drivers/scsi/aacraid/aachba.c   | 24 ++++++++++--------------
>>   drivers/scsi/aacraid/aacraid.h  |  2 +-
>>   drivers/scsi/aacraid/commctrl.c |  4 ++--
>>   drivers/scsi/aacraid/comminit.c |  3 +--
>>   4 files changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
>> index 03ba974f6b2a..b2849e5cc104 100644
>> --- a/drivers/scsi/aacraid/aachba.c
>> +++ b/drivers/scsi/aacraid/aachba.c
>> @@ -1336,8 +1336,7 @@ static int aac_read_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u32
>>   	if (ret < 0)
>>   		return ret;
>>   	fibsize = sizeof(struct aac_read) +
>> -			((le32_to_cpu(readcmd->sg.count) - 1) *
>> -			 sizeof (struct sgentry));
>> +		  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentry);
>>   	BUG_ON (fibsize > (fib->dev->max_fib_size -
>>   				sizeof(struct aac_fibhdr)));
>>   	/*
>> @@ -1471,8 +1470,7 @@ static int aac_write_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
>>   	if (ret < 0)
>>   		return ret;
>>   	fibsize = sizeof(struct aac_write) +
>> -		((le32_to_cpu(writecmd->sg.count) - 1) *
>> -		 sizeof (struct sgentry));
>> +		  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentry);
>>   	BUG_ON (fibsize > (fib->dev->max_fib_size -
>>   				sizeof(struct aac_fibhdr)));
>>   	/*
>> @@ -1590,9 +1588,9 @@ static int aac_scsi_64(struct fib * fib, struct scsi_cmnd * cmd)
>>   	/*
>>   	 *	Build Scatter/Gather list
>>   	 */
>> -	fibsize = sizeof (struct aac_srb) - sizeof (struct sgentry) +
>> -		((le32_to_cpu(srbcmd->sg.count) & 0xff) *
>> -		 sizeof (struct sgentry64));
>> +	fibsize = sizeof(struct aac_srb) +
>> +		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
>> +		  sizeof(struct sgentry64);
>>   	BUG_ON (fibsize > (fib->dev->max_fib_size -
>>   				sizeof(struct aac_fibhdr)));
>>   
>> @@ -1621,9 +1619,9 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
>>   	/*
>>   	 *	Build Scatter/Gather list
>>   	 */
>> -	fibsize = sizeof (struct aac_srb) +
>> -		(((le32_to_cpu(srbcmd->sg.count) & 0xff) - 1) *
>> -		 sizeof (struct sgentry));
>> +	fibsize = sizeof(struct aac_srb) +
>> +		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
>> +		  sizeof(struct sgentry);
>>   	BUG_ON (fibsize > (fib->dev->max_fib_size -
>>   				sizeof(struct aac_fibhdr)));
>>   
>> @@ -1691,8 +1689,7 @@ static int aac_send_safw_bmic_cmd(struct aac_dev *dev,
>>   	fibptr->hw_fib_va->header.XferState &=
>>   		~cpu_to_le32(FastResponseCapable);
>>   
>> -	fibsize  = sizeof(struct aac_srb) - sizeof(struct sgentry) +
>> -						sizeof(struct sgentry64);
>> +	fibsize  = sizeof(struct aac_srb) + sizeof(struct sgentry64);
>>   
>>   	/* allocate DMA buffer for response */
>>   	addr = dma_map_single(&dev->pdev->dev, xfer_buf, xfer_len,
>> @@ -2264,8 +2261,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
>>   		dev->a_ops.adapter_bounds = aac_bounds_32;
>>   		dev->scsi_host_ptr->sg_tablesize = (dev->max_fib_size -
>>   			sizeof(struct aac_fibhdr) -
>> -			sizeof(struct aac_write) + sizeof(struct sgentry)) /
>> -				sizeof(struct sgentry);
>> +			sizeof(struct aac_write)) / sizeof(struct sgentry);
>>   		if (dev->dac_support) {
>>   			dev->a_ops.adapter_read = aac_read_block64;
>>   			dev->a_ops.adapter_write = aac_write_block64;
>> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
>> index 94eb83d38be6..3fbc22ae72b6 100644
>> --- a/drivers/scsi/aacraid/aacraid.h
>> +++ b/drivers/scsi/aacraid/aacraid.h
>> @@ -507,7 +507,7 @@ struct sge_ieee1212 {
>>   
>>   struct sgmap {
>>   	__le32		count;
>> -	struct sgentry	sg[1];
>> +	struct sgentry	sg[];
>>   };
>>   
>>   struct user_sgmap {
>> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
>> index e7cc927ed952..df811ad4afaa 100644
>> --- a/drivers/scsi/aacraid/commctrl.c
>> +++ b/drivers/scsi/aacraid/commctrl.c
>> @@ -561,8 +561,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>>   		rcode = -EINVAL;
>>   		goto cleanup;
>>   	}
>> -	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
>> -		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
>> +	actual_fibsize = sizeof(struct aac_srb) +
>> +		(user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry);
>>   	actual_fibsize64 = actual_fibsize + (user_srbcmd->sg.count & 0xff) *
>>   	  (sizeof(struct sgentry64) - sizeof(struct sgentry));
>>   	/* User made a mistake - should not continue */
>> diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
>> index bd99c5492b7d..d8dd89c87b01 100644
>> --- a/drivers/scsi/aacraid/comminit.c
>> +++ b/drivers/scsi/aacraid/comminit.c
>> @@ -523,8 +523,7 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
>>   	dev->max_fib_size = sizeof(struct hw_fib);
>>   	dev->sg_tablesize = host->sg_tablesize = (dev->max_fib_size
>>   		- sizeof(struct aac_fibhdr)
>> -		- sizeof(struct aac_write) + sizeof(struct sgentry))
>> -			/ sizeof(struct sgentry);
>> +		- sizeof(struct aac_write)) / sizeof(struct sgentry);
>>   	dev->comm_interface = AAC_COMM_PRODUCER;
>>   	dev->raw_io_interface = dev->raw_io_64 = 0;
>>   
>> -- 
>> 2.34.1
>>
> 

[PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
sgmap64 and refactor the rest of the code, accordingly.

Issue found with the help of Coccinelle and audited and fixed,
manually.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aachba.c  | 9 +++------
 drivers/scsi/aacraid/aacraid.h | 2 +-
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index b2849e5cc104..90df697e7c5f 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1301,8 +1301,7 @@ static int aac_read_block64(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_read64) +
-		((le32_to_cpu(readcmd->sg.count) - 1) *
-		 sizeof (struct sgentry64));
+		  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentry64);
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 	/*
@@ -1433,8 +1432,7 @@ static int aac_write_block64(struct fib * fib, struct scsi_cmnd * cmd, u64 lba,
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_write64) +
-		((le32_to_cpu(writecmd->sg.count) - 1) *
-		 sizeof (struct sgentry64));
+		  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentry64);
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 	/*
@@ -2271,8 +2269,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 			dev->scsi_host_ptr->sg_tablesize =
 				(dev->max_fib_size -
 				sizeof(struct aac_fibhdr) -
-				sizeof(struct aac_write64) +
-				sizeof(struct sgentry64)) /
+				sizeof(struct aac_write64)) /
 					sizeof(struct sgentry64);
 		} else {
 			dev->a_ops.adapter_read = aac_read_block;
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 3fbc22ae72b6..fb3d93e4a99e 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -517,7 +517,7 @@ struct user_sgmap {
 
 struct sgmap64 {
 	__le32		count;
-	struct sgentry64 sg[1];
+	struct sgentry64 sg[];
 };
 
 struct user_sgmap64 {
-- 
2.34.1

Re: [PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64

[Kees Cook]

On Wed, Jun 28, 2023 at 11:57:30AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> sgmap64 and refactor the rest of the code, accordingly.
> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.

Like with the sgmap patch, I see (expected) binary differences in
aac_write_block64() and aac_read_block64() due to the simplified
calculations. I don't see anything unaccounted for, so:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/scsi/aacraid/aachba.c  | 9 +++------
>  drivers/scsi/aacraid/aacraid.h | 2 +-
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index b2849e5cc104..90df697e7c5f 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1301,8 +1301,7 @@ static int aac_read_block64(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
>  	if (ret < 0)
>  		return ret;
>  	fibsize = sizeof(struct aac_read64) +
> -		((le32_to_cpu(readcmd->sg.count) - 1) *
> -		 sizeof (struct sgentry64));
> +		  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentry64);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  	/*
> @@ -1433,8 +1432,7 @@ static int aac_write_block64(struct fib * fib, struct scsi_cmnd * cmd, u64 lba,
>  	if (ret < 0)
>  		return ret;
>  	fibsize = sizeof(struct aac_write64) +
> -		((le32_to_cpu(writecmd->sg.count) - 1) *
> -		 sizeof (struct sgentry64));
> +		  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentry64);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  	/*
> @@ -2271,8 +2269,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
>  			dev->scsi_host_ptr->sg_tablesize =
>  				(dev->max_fib_size -
>  				sizeof(struct aac_fibhdr) -
> -				sizeof(struct aac_write64) +
> -				sizeof(struct sgentry64)) /
> +				sizeof(struct aac_write64)) /
>  					sizeof(struct sgentry64);
>  		} else {
>  			dev->a_ops.adapter_read = aac_read_block;
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 3fbc22ae72b6..fb3d93e4a99e 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -517,7 +517,7 @@ struct user_sgmap {
>  
>  struct sgmap64 {
>  	__le32		count;
> -	struct sgentry64 sg[1];
> +	struct sgentry64 sg[];
>  };
>  
>  struct user_sgmap64 {
> -- 
> 2.34.1
> 

-- 
Kees Cook

[PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap

[Gustavo A. R. Silva]

Replace one-element array with flexible-array member in struct
user_sgmap and refactor the rest of the code, accordingly.

Issue found with the help of Coccinelle and audited and fixed,
manually.

This results in no differences in binary output.

Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/ClangBuiltLinux/linux/issues/1851
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/scsi/aacraid/aacraid.h  | 2 +-
 drivers/scsi/aacraid/commctrl.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index fb3d93e4a99e..7d3f2f7348ff 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -512,7 +512,7 @@ struct sgmap {
 
 struct user_sgmap {
 	u32		count;
-	struct user_sgentry	sg[1];
+	struct user_sgentry	sg[];
 };
 
 struct sgmap64 {
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index df811ad4afaa..74eb33eb70d3 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -523,7 +523,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		goto cleanup;
 	}
 
-	if ((fibsize < (sizeof(struct user_aac_srb) - sizeof(struct user_sgentry))) ||
+	if ((fibsize < sizeof(struct user_aac_srb)) ||
 	    (fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr)))) {
 		rcode = -EINVAL;
 		goto cleanup;
-- 
2.34.1

Re: [PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap

[Kees Cook]

On Wed, Jun 28, 2023 at 11:57:48AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> user_sgmap and refactor the rest of the code, accordingly.
> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.
> 
> This results in no differences in binary output.

Confirmed.

> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members

[Kees Cook]

On Wed, Jun 28, 2023 at 11:53:45AM -0600, Gustavo A. R. Silva wrote:
> This series aims to replace one-element arrays with flexible-array
> members in multiple structures in drivers/scsi/aacraid/aacraid.h.
> 
> 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().
> 
> These issues were found with the help of Coccinelle and audited and fixed,
> manually.
> 
> Link: https://github.com/KSPP/linux/issues/79
> 
> Gustavo A. R. Silva (10):
>   scsi: aacraid: Replace one-element array with flexible-array member
>   scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns()
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct aac_aifcmd
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct user_sgmapraw
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct sgmapraw
>   scsi: aacraid: Use struct_size() helper in code related to struct
>     sgmapraw
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct user_sgmap64
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct sgmap
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct sgmap64
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct user_sgmap

I'd like to reorganize this series so that all the conversions are
first, and then struct_size() additions are at the end. That way, if
desired, the conversions can land as fixes to turn the Clang builds
green again.

-- 
Kees Cook

Re: [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members

[Gustavo A. R. Silva]

On 6/28/23 14:08, Kees Cook wrote:
> On Wed, Jun 28, 2023 at 11:53:45AM -0600, Gustavo A. R. Silva wrote:
>> This series aims to replace one-element arrays with flexible-array
>> members in multiple structures in drivers/scsi/aacraid/aacraid.h.
>>
>> 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().
>>
>> These issues were found with the help of Coccinelle and audited and fixed,
>> manually.
>>
>> Link: https://github.com/KSPP/linux/issues/79
>>
>> Gustavo A. R. Silva (10):
>>    scsi: aacraid: Replace one-element array with flexible-array member
>>    scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns()
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct aac_aifcmd
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct user_sgmapraw
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct sgmapraw
>>    scsi: aacraid: Use struct_size() helper in code related to struct
>>      sgmapraw
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct user_sgmap64
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct sgmap
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct sgmap64
>>    scsi: aacraid: Replace one-element array with flexible-array member in
>>      struct user_sgmap
> 
> I'd like to reorganize this series so that all the conversions are
> first, and then struct_size() additions are at the end. That way, if
> desired, the conversions can land as fixes to turn the Clang builds
> green again.
> 

OK; I can make that happen. :)

--
Gustavo