* [PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
@ 2023-06-28 17:54 ` Gustavo A. R. Silva
2023-06-28 20:09 ` Kees Cook
2023-06-28 17:54 ` [PATCH 02/10][next] scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns() Gustavo A. R. Silva
` (9 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:54 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member
2023-06-28 17:54 ` [PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member Gustavo A. R. Silva
@ 2023-06-28 20:09 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:09 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 02/10][next] scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns()
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
2023-06-28 17:54 ` [PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member Gustavo A. R. Silva
@ 2023-06-28 17:54 ` Gustavo A. R. Silva
2023-06-28 20:51 ` Kees Cook
2023-06-28 17:55 ` [PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd Gustavo A. R. Silva
` (8 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:54 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
2023-06-28 17:54 ` [PATCH 01/10][next] scsi: aacraid: Replace one-element array with flexible-array member Gustavo A. R. Silva
2023-06-28 17:54 ` [PATCH 02/10][next] scsi: aacraid: Use struct_size() helper in aac_get_safw_ciss_luns() Gustavo A. R. Silva
@ 2023-06-28 17:55 ` Gustavo A. R. Silva
2023-06-28 20:10 ` Kees Cook
2023-06-28 17:55 ` [PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw Gustavo A. R. Silva
` (7 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:55 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd
2023-06-28 17:55 ` [PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd Gustavo A. R. Silva
@ 2023-06-28 20:10 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:10 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (2 preceding siblings ...)
2023-06-28 17:55 ` [PATCH 03/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct aac_aifcmd Gustavo A. R. Silva
@ 2023-06-28 17:55 ` Gustavo A. R. Silva
2023-06-28 20:10 ` Kees Cook
2023-06-28 17:56 ` [PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw Gustavo A. R. Silva
` (6 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:55 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw
2023-06-28 17:55 ` [PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw Gustavo A. R. Silva
@ 2023-06-28 20:10 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:10 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (3 preceding siblings ...)
2023-06-28 17:55 ` [PATCH 04/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmapraw Gustavo A. R. Silva
@ 2023-06-28 17:56 ` Gustavo A. R. Silva
2023-06-28 20:49 ` Kees Cook
2023-06-28 17:56 ` [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to " Gustavo A. R. Silva
` (5 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:56 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw
2023-06-28 17:56 ` [PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw Gustavo A. R. Silva
@ 2023-06-28 20:49 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:49 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to struct sgmapraw
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (4 preceding siblings ...)
2023-06-28 17:56 ` [PATCH 05/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmapraw Gustavo A. R. Silva
@ 2023-06-28 17:56 ` Gustavo A. R. Silva
2023-06-28 20:51 ` Kees Cook
2023-06-28 20:52 ` Kees Cook
2023-06-28 17:56 ` [PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64 Gustavo A. R. Silva
` (4 subsequent siblings)
10 siblings, 2 replies; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:56 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to struct sgmapraw
2023-06-28 17:56 ` [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to " Gustavo A. R. Silva
@ 2023-06-28 20:51 ` Kees Cook
2023-06-28 20:52 ` Kees Cook
1 sibling, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:51 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to struct sgmapraw
2023-06-28 17:56 ` [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to " Gustavo A. R. Silva
2023-06-28 20:51 ` Kees Cook
@ 2023-06-28 20:52 ` Kees Cook
1 sibling, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:52 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (5 preceding siblings ...)
2023-06-28 17:56 ` [PATCH 06/10][next] scsi: aacraid: Use struct_size() helper in code related to " Gustavo A. R. Silva
@ 2023-06-28 17:56 ` Gustavo A. R. Silva
2023-06-28 20:10 ` Kees Cook
2023-06-28 17:57 ` [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap Gustavo A. R. Silva
` (3 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:56 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64
2023-06-28 17:56 ` [PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64 Gustavo A. R. Silva
@ 2023-06-28 20:10 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:10 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (6 preceding siblings ...)
2023-06-28 17:56 ` [PATCH 07/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap64 Gustavo A. R. Silva
@ 2023-06-28 17:57 ` Gustavo A. R. Silva
2023-06-28 20:36 ` Kees Cook
2023-06-28 17:57 ` [PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64 Gustavo A. R. Silva
` (2 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:57 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap
2023-06-28 17:57 ` [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap Gustavo A. R. Silva
@ 2023-06-28 20:36 ` Kees Cook
2023-06-28 21:15 ` Gustavo A. R. Silva
0 siblings, 1 reply; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:36 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap
2023-06-28 20:36 ` Kees Cook
@ 2023-06-28 21:15 ` Gustavo A. R. Silva
0 siblings, 0 replies; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 21:15 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (7 preceding siblings ...)
2023-06-28 17:57 ` [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap Gustavo A. R. Silva
@ 2023-06-28 17:57 ` Gustavo A. R. Silva
2023-06-28 20:46 ` Kees Cook
2023-06-28 17:57 ` [PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap Gustavo A. R. Silva
2023-06-28 20:08 ` [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Kees Cook
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:57 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64
2023-06-28 17:57 ` [PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64 Gustavo A. R. Silva
@ 2023-06-28 20:46 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:46 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (8 preceding siblings ...)
2023-06-28 17:57 ` [PATCH 09/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap64 Gustavo A. R. Silva
@ 2023-06-28 17:57 ` Gustavo A. R. Silva
2023-06-28 20:11 ` Kees Cook
2023-06-28 20:08 ` [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Kees Cook
10 siblings, 1 reply; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 17:57 UTC (permalink / raw)
To: aacraid, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Kees Cook, Gustavo A. R. Silva,
linux-hardening
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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap
2023-06-28 17:57 ` [PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap Gustavo A. R. Silva
@ 2023-06-28 20:11 ` Kees Cook
0 siblings, 0 replies; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:11 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members
2023-06-28 17:53 [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
` (9 preceding siblings ...)
2023-06-28 17:57 ` [PATCH 10/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct user_sgmap Gustavo A. R. Silva
@ 2023-06-28 20:08 ` Kees Cook
2023-06-28 20:16 ` Gustavo A. R. Silva
10 siblings, 1 reply; 25+ messages in thread
From: Kees Cook @ 2023-06-28 20:08 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members
2023-06-28 20:08 ` [PATCH 00/10][next] scsi: aacraid: Replace one-element arrays with flexible-array members Kees Cook
@ 2023-06-28 20:16 ` Gustavo A. R. Silva
0 siblings, 0 replies; 25+ messages in thread
From: Gustavo A. R. Silva @ 2023-06-28 20:16 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: aacraid, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 25+ messages in thread