Re: [PATCH][next] media: siano: Fix multiple out-of-bounds warnings in smscore_load_firmware_family2()
From: Gustavo A. R. Silva
Date: Fri Mar 26 2021 - 13:52:43 EST
Hi all,
Friendly ping: who can take this, please?
Thanks
--
Gustavo
On 3/10/21 20:19, 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>
> ---
> 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 {
>