diff mbox series

scsi: megaraid_sas: struct MR_HOST_DEVICE_LIST: Replace 1-element array with flexible array

Message ID 20240711155841.work.839-kees@kernel.org (mailing list archive)
State Accepted
Headers show
Series scsi: megaraid_sas: struct MR_HOST_DEVICE_LIST: Replace 1-element array with flexible array | expand

Commit Message

Kees Cook July 11, 2024, 3:58 p.m. UTC
Replace the deprecated[1] use of a 1-element array in
struct MR_HOST_DEVICE_LIST with a modern flexible array.

One binary difference appears in megasas_host_device_list_query():

        struct MR_HOST_DEVICE_LIST *ci;
	...
        ci = instance->host_device_list_buf;
	...
        memset(ci, 0, sizeof(*ci));

The memset() clears only the non-flexible array fields. Looking at the
rest of the function, this appears to be fine: firmware is using this
region to communicate with the kernel, so it likely never made sense to
clear the first MR_HOST_DEVICE_LIST_ENTRY.

Link: https://github.com/KSPP/linux/issues/79 [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Cc: Chandrakanth patil <chandrakanth.patil@broadcom.com>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: megaraidlinux.pdl@broadcom.com
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/megaraid/megaraid_sas.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Gustavo A. R. Silva July 11, 2024, 4:55 p.m. UTC | #1
On 11/07/24 09:58, Kees Cook wrote:
> Replace the deprecated[1] use of a 1-element array in
> struct MR_HOST_DEVICE_LIST with a modern flexible array.
> 
> One binary difference appears in megasas_host_device_list_query():
> 
>          struct MR_HOST_DEVICE_LIST *ci;
> 	...
>          ci = instance->host_device_list_buf;
> 	...
>          memset(ci, 0, sizeof(*ci));
> 
> The memset() clears only the non-flexible array fields. Looking at the
> rest of the function, this appears to be fine: firmware is using this
> region to communicate with the kernel, so it likely never made sense to
> clear the first MR_HOST_DEVICE_LIST_ENTRY.

Yeah, clearing that fist entry seems odd/buggy. So, this patch is probably
even fixing a bug. :)

> 
> Link: https://github.com/KSPP/linux/issues/79 [1]
> Signed-off-by: Kees Cook <kees@kernel.org>

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

Thanks
James Bottomley July 11, 2024, 6:11 p.m. UTC | #2
On Thu, 2024-07-11 at 08:58 -0700, Kees Cook wrote:
> Replace the deprecated[1] use of a 1-element array in
> struct MR_HOST_DEVICE_LIST with a modern flexible array.
> 
> One binary difference appears in megasas_host_device_list_query():
> 
>         struct MR_HOST_DEVICE_LIST *ci;
>         ...
>         ci = instance->host_device_list_buf;
>         ...
>         memset(ci, 0, sizeof(*ci));
> 
> The memset() clears only the non-flexible array fields. Looking at
> the rest of the function, this appears to be fine: firmware is using
> this region to communicate with the kernel, so it likely never made
> sense to clear the first MR_HOST_DEVICE_LIST_ENTRY.

That's not necessarily a safe assumption: older qlogic for instance
uses zeroing an entry to stop the card mailbox processing.  Looking at
the driver, I think you're right: it's only used for card to host
communication, so clearing it is irrelevant, but it could be relevant
if it were also used for host to card communication.

Regards,

James
Martin K. Petersen Aug. 3, 2024, 1:27 a.m. UTC | #3
Kees,

> Replace the deprecated[1] use of a 1-element array in struct
> MR_HOST_DEVICE_LIST with a modern flexible array.

Applied to 6.12/scsi-staging, thanks!
diff mbox series

Patch

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index 84cf77c48c0d..088cc40ae866 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -814,12 +814,12 @@  struct MR_HOST_DEVICE_LIST {
 	__le32			size;
 	__le32			count;
 	__le32			reserved[2];
-	struct MR_HOST_DEVICE_LIST_ENTRY	host_device_list[1];
+	struct MR_HOST_DEVICE_LIST_ENTRY	host_device_list[] __counted_by_le(count);
 } __packed;
 
 #define HOST_DEVICE_LIST_SZ (sizeof(struct MR_HOST_DEVICE_LIST) +	       \
 			      (sizeof(struct MR_HOST_DEVICE_LIST_ENTRY) *      \
-			      (MEGASAS_MAX_PD + MAX_LOGICAL_DRIVES_EXT - 1)))
+			      (MEGASAS_MAX_PD + MAX_LOGICAL_DRIVES_EXT)))
 
 
 /*