Re: [PATCH RESEND][next] media: siano: Fix out-of-bounds warnings in smscore_load_firmware_family2()
From: Gustavo A. R. Silva
Date: Tue Jun 01 2021 - 18:11:38 EST
Hi,
I've been pinging and resending this patch multiple times. Is there someone out there
that can give me some feedback on this, please?
I'm fine with adding this to my -next branch for 5.14 if no one cares. :)
Thanks
--
Gustavo
On 5/11/21 12:45, Gustavo A. R. Silva wrote:
> Rename struct sms_msg_data4 to sms_msg_data5 and increase the size of
> its msg_data array from 4 to 5 elements. Notice that at some point
> the 5th element of msg_data is being accessed in function
> smscore_load_firmware_family2():
>
> 1006 trigger_msg->msg_data[4] = 4; /* Task ID */
>
> Also, there is no need for the object _trigger_msg_ of type struct
> sms_msg_data *, when _msg_ can be used, directly. Notice that msg_data
> in struct sms_msg_data is a one-element array, which causes multiple
> out-of-bounds warnings when accessing beyond its first element
> in function smscore_load_firmware_family2():
>
> 992 struct sms_msg_data *trigger_msg =
> 993 (struct sms_msg_data *) msg;
> 994
> 995 pr_debug("sending MSG_SMS_SWDOWNLOAD_TRIGGER_REQ\n");
> 996 SMS_INIT_MSG(&msg->x_msg_header,
> 997 MSG_SMS_SWDOWNLOAD_TRIGGER_REQ,
> 998 sizeof(struct sms_msg_hdr) +
> 999 sizeof(u32) * 5);
> 1000
> 1001 trigger_msg->msg_data[0] = firmware->start_address;
> 1002 /* Entry point */
> 1003 trigger_msg->msg_data[1] = 6; /* Priority */
> 1004 trigger_msg->msg_data[2] = 0x200; /* Stack size */
> 1005 trigger_msg->msg_data[3] = 0; /* Parameter */
> 1006 trigger_msg->msg_data[4] = 4; /* Task ID */
>
> even when enough dynamic memory is allocated for _msg_:
>
> 929 /* PAGE_SIZE buffer shall be enough and dma aligned */
> 930 msg = kmalloc(PAGE_SIZE, GFP_KERNEL | coredev->gfp_buf_flags);
>
> but as _msg_ is casted to (struct sms_msg_data *):
>
> 992 struct sms_msg_data *trigger_msg =
> 993 (struct sms_msg_data *) msg;
>
> the out-of-bounds warnings are actually valid and should be addressed.
>
> Fix this by declaring object _msg_ of type struct sms_msg_data5 *,
> which contains a 5-elements array, instead of just 4. And use
> _msg_ directly, instead of creating object trigger_msg.
>
> This helps with the ongoing efforts to enable -Warray-bounds by fixing
> the following warnings:
>
> CC [M] drivers/media/common/siano/smscoreapi.o
> drivers/media/common/siano/smscoreapi.c: In function ‘smscore_load_firmware_family2’:
> drivers/media/common/siano/smscoreapi.c:1003:24: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 1003 | trigger_msg->msg_data[1] = 6; /* Priority */
> | ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
> 619 | u32 msg_data[1];
> | ^~~~~~~~
> drivers/media/common/siano/smscoreapi.c:1004:24: warning: array subscript 2 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 1004 | trigger_msg->msg_data[2] = 0x200; /* Stack size */
> | ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
> 619 | u32 msg_data[1];
> | ^~~~~~~~
> drivers/media/common/siano/smscoreapi.c:1005:24: warning: array subscript 3 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 1005 | trigger_msg->msg_data[3] = 0; /* Parameter */
> | ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
> 619 | u32 msg_data[1];
> | ^~~~~~~~
> drivers/media/common/siano/smscoreapi.c:1006:24: warning: array subscript 4 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 1006 | trigger_msg->msg_data[4] = 4; /* Task ID */
> | ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
> 619 | u32 msg_data[1];
> | ^~~~~~~~
>
> Fixes: 018b0c6f8acb ("[media] siano: make load firmware logic to work with newer firmwares")
> Co-developed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
> ---
> Hi,
>
> We are about to be able to globally enable -Warray-bounds and,
> these are pretty much the last out-of-bounds warnings in linux-next.
>
> Thanks
>
> drivers/media/common/siano/smscoreapi.c | 22 +++++++++-------------
> drivers/media/common/siano/smscoreapi.h | 4 ++--
> 2 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c
> index 410cc3ac6f94..bceaf91faa15 100644
> --- a/drivers/media/common/siano/smscoreapi.c
> +++ b/drivers/media/common/siano/smscoreapi.c
> @@ -908,7 +908,7 @@ static int smscore_load_firmware_family2(struct smscore_device_t *coredev,
> void *buffer, size_t size)
> {
> struct sms_firmware *firmware = (struct sms_firmware *) buffer;
> - struct sms_msg_data4 *msg;
> + struct sms_msg_data5 *msg;
> u32 mem_address, calc_checksum = 0;
> u32 i, *ptr;
> u8 *payload = firmware->payload;
> @@ -989,24 +989,20 @@ static int smscore_load_firmware_family2(struct smscore_device_t *coredev,
> goto exit_fw_download;
>
> if (coredev->mode == DEVICE_MODE_NONE) {
> - struct sms_msg_data *trigger_msg =
> - (struct sms_msg_data *) msg;
> -
> pr_debug("sending MSG_SMS_SWDOWNLOAD_TRIGGER_REQ\n");
> SMS_INIT_MSG(&msg->x_msg_header,
> MSG_SMS_SWDOWNLOAD_TRIGGER_REQ,
> - sizeof(struct sms_msg_hdr) +
> - sizeof(u32) * 5);
> + sizeof(*msg));
>
> - trigger_msg->msg_data[0] = firmware->start_address;
> + msg->msg_data[0] = firmware->start_address;
> /* Entry point */
> - trigger_msg->msg_data[1] = 6; /* Priority */
> - trigger_msg->msg_data[2] = 0x200; /* Stack size */
> - trigger_msg->msg_data[3] = 0; /* Parameter */
> - trigger_msg->msg_data[4] = 4; /* Task ID */
> + msg->msg_data[1] = 6; /* Priority */
> + msg->msg_data[2] = 0x200; /* Stack size */
> + msg->msg_data[3] = 0; /* Parameter */
> + msg->msg_data[4] = 4; /* Task ID */
>
> - rc = smscore_sendrequest_and_wait(coredev, trigger_msg,
> - trigger_msg->x_msg_header.msg_length,
> + rc = smscore_sendrequest_and_wait(coredev, msg,
> + msg->x_msg_header.msg_length,
> &coredev->trigger_done);
> } else {
> SMS_INIT_MSG(&msg->x_msg_header, MSG_SW_RELOAD_EXEC_REQ,
> diff --git a/drivers/media/common/siano/smscoreapi.h b/drivers/media/common/siano/smscoreapi.h
> index 4a6b9f4c44ac..f8789ee0d554 100644
> --- a/drivers/media/common/siano/smscoreapi.h
> +++ b/drivers/media/common/siano/smscoreapi.h
> @@ -624,9 +624,9 @@ struct sms_msg_data2 {
> u32 msg_data[2];
> };
>
> -struct sms_msg_data4 {
> +struct sms_msg_data5 {
> struct sms_msg_hdr x_msg_header;
> - u32 msg_data[4];
> + u32 msg_data[5];
> };
>
> struct sms_data_download {
>