| Message ID | 20240711155841.work.839-kees@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 29b4a49750776925ea48ef440da6aa75828913db |
| Headers | show |
| Series | scsi: megaraid_sas: struct MR_HOST_DEVICE_LIST: Replace 1-element array with flexible array | expand |
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
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
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 --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))) /*
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(-)