* [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3
@ 2021-02-02 23:51 Gustavo A. R. Silva
2021-03-08 19:32 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-02-02 23:51 UTC (permalink / raw)
To: Sathya Prakash, Sreekanth Reddy, Suganath Prabu Subramani,
James E.J. Bottomley, Martin K. Petersen
Cc: MPT-FusionLinux.pdl, linux-scsi, linux-kernel,
Gustavo A. R. Silva, linux-hardening
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct _MPI2_CONFIG_PAGE_IO_UNIT_3, instead of a one-element array,
and use the struct_size() helper to calculate the size for the
allocation.
Also, this helps the ongoing efforts to enable -Warray-bounds and fix the
following warnings:
drivers/scsi/mpt3sas/mpt3sas_ctl.c:3193:63: warning: array subscript 24
is above array bounds of ‘U16[1]’ {aka ‘short unsigned int[1]’}
[-Warray-bounds]
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
- Fix format specifier: use %zu for size_t type.
drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 11 +----------
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 6 +++---
2 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
index 43a3bf8ff428..908b0ca63204 100644
--- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
+++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
@@ -987,21 +987,12 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 {
/*IO Unit Page 3 */
-/*
- *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
- *one and check the value returned for GPIOCount at runtime.
- */
-#ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX
-#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1)
-#endif
-
typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 {
MPI2_CONFIG_PAGE_HEADER Header; /*0x00 */
U8 GPIOCount; /*0x04 */
U8 Reserved1; /*0x05 */
U16 Reserved2; /*0x06 */
- U16
- GPIOVal[MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX];/*0x08 */
+ U16 GPIOVal[]; /*0x08 */
} MPI2_CONFIG_PAGE_IO_UNIT_3,
*PTR_MPI2_CONFIG_PAGE_IO_UNIT_3,
Mpi2IOUnitPage3_t, *pMpi2IOUnitPage3_t;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index c8a0ce18f2c5..ffb21f873058 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -3143,7 +3143,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
Mpi2ConfigReply_t mpi_reply;
u16 backup_rail_monitor_status = 0;
u16 ioc_status;
- int sz;
+ size_t sz;
ssize_t rc = 0;
if (!ioc->is_warpdrive) {
@@ -3157,11 +3157,11 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
goto out;
/* allocate upto GPIOVal 36 entries */
- sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
+ sz = struct_size(io_unit_pg3, GPIOVal, 36);
io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
if (!io_unit_pg3) {
rc = -ENOMEM;
- ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
+ ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%zu) bytes\n",
__func__, sz);
goto out;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3
2021-02-02 23:51 [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3 Gustavo A. R. Silva
@ 2021-03-08 19:32 ` Gustavo A. R. Silva
2021-03-08 20:12 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-08 19:32 UTC (permalink / raw)
To: Sathya Prakash, Sreekanth Reddy, Suganath Prabu Subramani,
James E.J. Bottomley, Martin K. Petersen
Cc: MPT-FusionLinux.pdl, linux-scsi, linux-kernel, linux-hardening
Hi all,
Friendly ping: who can review/take this, please?
Thanks!
--
Gustavo
On Tue, Feb 02, 2021 at 05:51:18PM -0600, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
>
> Refactor the code according to the use of a flexible-array member in
> struct _MPI2_CONFIG_PAGE_IO_UNIT_3, instead of a one-element array,
> and use the struct_size() helper to calculate the size for the
> allocation.
>
> Also, this helps the ongoing efforts to enable -Warray-bounds and fix the
> following warnings:
>
> drivers/scsi/mpt3sas/mpt3sas_ctl.c:3193:63: warning: array subscript 24
> is above array bounds of ‘U16[1]’ {aka ‘short unsigned int[1]’}
> [-Warray-bounds]
>
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/109
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Changes in v2:
> - Fix format specifier: use %zu for size_t type.
>
> drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 11 +----------
> drivers/scsi/mpt3sas/mpt3sas_ctl.c | 6 +++---
> 2 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
> index 43a3bf8ff428..908b0ca63204 100644
> --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
> @@ -987,21 +987,12 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 {
>
> /*IO Unit Page 3 */
>
> -/*
> - *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
> - *one and check the value returned for GPIOCount at runtime.
> - */
> -#ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX
> -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1)
> -#endif
> -
> typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 {
> MPI2_CONFIG_PAGE_HEADER Header; /*0x00 */
> U8 GPIOCount; /*0x04 */
> U8 Reserved1; /*0x05 */
> U16 Reserved2; /*0x06 */
> - U16
> - GPIOVal[MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX];/*0x08 */
> + U16 GPIOVal[]; /*0x08 */
> } MPI2_CONFIG_PAGE_IO_UNIT_3,
> *PTR_MPI2_CONFIG_PAGE_IO_UNIT_3,
> Mpi2IOUnitPage3_t, *pMpi2IOUnitPage3_t;
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index c8a0ce18f2c5..ffb21f873058 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -3143,7 +3143,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
> Mpi2ConfigReply_t mpi_reply;
> u16 backup_rail_monitor_status = 0;
> u16 ioc_status;
> - int sz;
> + size_t sz;
> ssize_t rc = 0;
>
> if (!ioc->is_warpdrive) {
> @@ -3157,11 +3157,11 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
> goto out;
>
> /* allocate upto GPIOVal 36 entries */
> - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
> + sz = struct_size(io_unit_pg3, GPIOVal, 36);
> io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
> if (!io_unit_pg3) {
> rc = -ENOMEM;
> - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
> + ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%zu) bytes\n",
> __func__, sz);
> goto out;
> }
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3
2021-03-08 19:32 ` Gustavo A. R. Silva
@ 2021-03-08 20:12 ` James Bottomley
2021-03-08 20:41 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2021-03-08 20:12 UTC (permalink / raw)
To: Gustavo A. R. Silva, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, Martin K. Petersen
Cc: MPT-FusionLinux.pdl, linux-scsi, linux-kernel, linux-hardening
On Mon, 2021-03-08 at 13:32 -0600, Gustavo A. R. Silva wrote:
> Hi all,
>
> Friendly ping: who can review/take this, please?
Well, before embarking on a huge dynamic update, let's ask Broadcom the
simpler question of why isn't MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX simply
set to 36? There's no dynamic allocation of anything in the current
code ... it's all hard coded to allocate 36 entries. If there's no
need for anything dynamic then the kzalloc could become
io_unit_pg3 = kzalloc(sizeof(*io_unit_pg3), GFP_KERNEL);
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3
2021-03-08 20:12 ` James Bottomley
@ 2021-03-08 20:41 ` Gustavo A. R. Silva
2021-03-10 19:07 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-08 20:41 UTC (permalink / raw)
To: James Bottomley
Cc: Sathya Prakash, Sreekanth Reddy, Suganath Prabu Subramani,
Martin K. Petersen, MPT-FusionLinux.pdl, linux-scsi,
linux-kernel, linux-hardening
On Mon, Mar 08, 2021 at 12:12:59PM -0800, James Bottomley wrote:
> On Mon, 2021-03-08 at 13:32 -0600, Gustavo A. R. Silva wrote:
> > Hi all,
> >
> > Friendly ping: who can review/take this, please?
>
> Well, before embarking on a huge dynamic update, let's ask Broadcom the
> simpler question of why isn't MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX simply
> set to 36? There's no dynamic allocation of anything in the current
> code ... it's all hard coded to allocate 36 entries. If there's no
> need for anything dynamic then the kzalloc could become
Yeah; and if that is the case, then there is no even need for kzalloc()
at all, and it can be replaced by memset():
diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
index 43a3bf8ff428..d00431f553e1 100644
--- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
+++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
@@ -992,7 +992,7 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 {
*one and check the value returned for GPIOCount at runtime.
*/
#ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX
-#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1)
+#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (36)
#endif
typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 {
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 44f9a05db94e..23fcf29bfd67 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -3203,7 +3203,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
{
struct Scsi_Host *shost = class_to_shost(cdev);
struct MPT3SAS_ADAPTER *ioc = shost_priv(shost);
- Mpi2IOUnitPage3_t *io_unit_pg3 = NULL;
+ Mpi2IOUnitPage3_t io_unit_pg3;
Mpi2ConfigReply_t mpi_reply;
u16 backup_rail_monitor_status = 0;
u16 ioc_status;
@@ -3221,16 +3221,10 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
goto out;
/* allocate upto GPIOVal 36 entries */
- sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
- io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
- if (!io_unit_pg3) {
- rc = -ENOMEM;
- ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
- __func__, sz);
- goto out;
- }
+ sz = sizeof(io_unit_pg3);
+ memset(&io_unit_pg3, 0, sz);
- if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) !=
+ if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, &io_unit_pg3, sz) !=
0) {
ioc_err(ioc, "%s: failed reading iounit_pg3\n",
__func__);
@@ -3246,19 +3240,18 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
goto out;
}
- if (io_unit_pg3->GPIOCount < 25) {
- ioc_err(ioc, "%s: iounit_pg3->GPIOCount less than 25 entries, detected (%d) entries\n",
- __func__, io_unit_pg3->GPIOCount);
+ if (io_unit_pg3.GPIOCount < 25) {
+ ioc_err(ioc, "%s: iounit_pg3.GPIOCount less than 25 entries, detected (%d) entries\n",
+ __func__, io_unit_pg3.GPIOCount);
rc = -EINVAL;
goto out;
}
/* BRM status is in bit zero of GPIOVal[24] */
- backup_rail_monitor_status = le16_to_cpu(io_unit_pg3->GPIOVal[24]);
+ backup_rail_monitor_status = le16_to_cpu(io_unit_pg3.GPIOVal[24]);
rc = snprintf(buf, PAGE_SIZE, "%d\n", (backup_rail_monitor_status & 1));
out:
- kfree(io_unit_pg3);
mutex_unlock(&ioc->pci_access_mutex);
return rc;
}
>
> io_unit_pg3 = kzalloc(sizeof(*io_unit_pg3), GFP_KERNEL);
>
Thanks
--
Gustavo
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3
2021-03-08 20:41 ` Gustavo A. R. Silva
@ 2021-03-10 19:07 ` Kees Cook
2021-03-11 0:06 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2021-03-10 19:07 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: James Bottomley, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, Martin K. Petersen,
MPT-FusionLinux.pdl, linux-scsi, linux-kernel, linux-hardening
On Mon, Mar 08, 2021 at 02:41:29PM -0600, Gustavo A. R. Silva wrote:
> On Mon, Mar 08, 2021 at 12:12:59PM -0800, James Bottomley wrote:
> > On Mon, 2021-03-08 at 13:32 -0600, Gustavo A. R. Silva wrote:
> > > Hi all,
> > >
> > > Friendly ping: who can review/take this, please?
> >
> > Well, before embarking on a huge dynamic update, let's ask Broadcom the
> > simpler question of why isn't MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX simply
> > set to 36? There's no dynamic allocation of anything in the current
> > code ... it's all hard coded to allocate 36 entries. If there's no
> > need for anything dynamic then the kzalloc could become
>
> Yeah; and if that is the case, then there is no even need for kzalloc()
> at all, and it can be replaced by memset():
>
> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
> index 43a3bf8ff428..d00431f553e1 100644
> --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
> @@ -992,7 +992,7 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 {
> *one and check the value returned for GPIOCount at runtime.
> */
> #ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX
> -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1)
> +#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (36)
> #endif
>
> typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 {
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 44f9a05db94e..23fcf29bfd67 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -3203,7 +3203,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
> {
> struct Scsi_Host *shost = class_to_shost(cdev);
> struct MPT3SAS_ADAPTER *ioc = shost_priv(shost);
> - Mpi2IOUnitPage3_t *io_unit_pg3 = NULL;
> + Mpi2IOUnitPage3_t io_unit_pg3;
> Mpi2ConfigReply_t mpi_reply;
> u16 backup_rail_monitor_status = 0;
> u16 ioc_status;
> @@ -3221,16 +3221,10 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
> goto out;
>
> /* allocate upto GPIOVal 36 entries */
> - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
> - io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
> - if (!io_unit_pg3) {
> - rc = -ENOMEM;
> - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
> - __func__, sz);
> - goto out;
> - }
> + sz = sizeof(io_unit_pg3);
> + memset(&io_unit_pg3, 0, sz);
I like this a lot. It makes the code way simpler.
Putting this on the stack makes it faster, and it's less than 100 bytes,
which seems entirely reasonable.
>
> - if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) !=
> + if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, &io_unit_pg3, sz) !=
The only thing I can imagine is if this ends up doing DMA, which isn't
allowed on the stack. However, in looking down through the call path,
it's _copied_ into DMA memory, so this appears entirely safe.
Can you send this as a "normal" patch? Feel free to include:
Reviewed-by: Kees Cook <keescook@chromium.org>
Thanks!
-Kees
> 0) {
> ioc_err(ioc, "%s: failed reading iounit_pg3\n",
> __func__);
> @@ -3246,19 +3240,18 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
> goto out;
> }
>
> - if (io_unit_pg3->GPIOCount < 25) {
> - ioc_err(ioc, "%s: iounit_pg3->GPIOCount less than 25 entries, detected (%d) entries\n",
> - __func__, io_unit_pg3->GPIOCount);
> + if (io_unit_pg3.GPIOCount < 25) {
> + ioc_err(ioc, "%s: iounit_pg3.GPIOCount less than 25 entries, detected (%d) entries\n",
> + __func__, io_unit_pg3.GPIOCount);
> rc = -EINVAL;
> goto out;
> }
>
> /* BRM status is in bit zero of GPIOVal[24] */
> - backup_rail_monitor_status = le16_to_cpu(io_unit_pg3->GPIOVal[24]);
> + backup_rail_monitor_status = le16_to_cpu(io_unit_pg3.GPIOVal[24]);
> rc = snprintf(buf, PAGE_SIZE, "%d\n", (backup_rail_monitor_status & 1));
>
> out:
> - kfree(io_unit_pg3);
> mutex_unlock(&ioc->pci_access_mutex);
> return rc;
> }
>
> >
> > io_unit_pg3 = kzalloc(sizeof(*io_unit_pg3), GFP_KERNEL);
> >
>
> Thanks
> --
> Gustavo
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3
2021-03-10 19:07 ` Kees Cook
@ 2021-03-11 0:06 ` Gustavo A. R. Silva
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-11 0:06 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: James Bottomley, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, Martin K. Petersen,
MPT-FusionLinux.pdl, linux-scsi, linux-kernel, linux-hardening
On 3/10/21 13:07, Kees Cook wrote:
>> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
>> index 43a3bf8ff428..d00431f553e1 100644
>> --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
>> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
>> @@ -992,7 +992,7 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 {
>> *one and check the value returned for GPIOCount at runtime.
>> */
>> #ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX
>> -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1)
>> +#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (36)
>> #endif
>>
>> typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 {
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> index 44f9a05db94e..23fcf29bfd67 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> @@ -3203,7 +3203,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
>> {
>> struct Scsi_Host *shost = class_to_shost(cdev);
>> struct MPT3SAS_ADAPTER *ioc = shost_priv(shost);
>> - Mpi2IOUnitPage3_t *io_unit_pg3 = NULL;
>> + Mpi2IOUnitPage3_t io_unit_pg3;
>> Mpi2ConfigReply_t mpi_reply;
>> u16 backup_rail_monitor_status = 0;
>> u16 ioc_status;
>> @@ -3221,16 +3221,10 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
>> goto out;
>>
>> /* allocate upto GPIOVal 36 entries */
>> - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
>> - io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
>> - if (!io_unit_pg3) {
>> - rc = -ENOMEM;
>> - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
>> - __func__, sz);
>> - goto out;
>> - }
>> + sz = sizeof(io_unit_pg3);
>> + memset(&io_unit_pg3, 0, sz);
>
> I like this a lot. It makes the code way simpler.
>
> Putting this on the stack makes it faster, and it's less than 100 bytes,
> which seems entirely reasonable.
>
>>
>> - if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) !=
>> + if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, &io_unit_pg3, sz) !=
>
> The only thing I can imagine is if this ends up doing DMA, which isn't
> allowed on the stack. However, in looking down through the call path,
> it's _copied_ into DMA memory, so this appears entirely safe.
>
> Can you send this as a "normal" patch? Feel free to include:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
Done: https://lore.kernel.org/lkml/20210310235951.GA108661@embeddedor/
Thanks for the comments!
--
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-11 0:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 23:51 [PATCH v2][next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3 Gustavo A. R. Silva
2021-03-08 19:32 ` Gustavo A. R. Silva
2021-03-08 20:12 ` James Bottomley
2021-03-08 20:41 ` Gustavo A. R. Silva
2021-03-10 19:07 ` Kees Cook
2021-03-11 0:06 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.