From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To: "Gustavo A. R. Silva" <gustavoars@kernel.org>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Kees Cook <keescook@chromium.org>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-hardening@vger.kernel.org
Subject: Re: [PATCH][next] media: siano: Fix multiple out-of-bounds warnings in smscore_load_firmware_family2()
Date: Mon, 12 Apr 2021 10:00:32 -0500 [thread overview]
Message-ID: <91073669-7a9b-9909-baa5-d4ddd814aa35@embeddedor.com> (raw)
In-Reply-To: <b0e46b46-db9a-7e3d-cadb-e3855c68373e@embeddedor.com>
Hi all,
Friendly ping: who can take this, please?
Thanks
--
Gustavo
On 3/26/21 11:30, Gustavo A. R. Silva wrote:
> 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@chromium.org>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> 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 {
>>
next prev parent reply other threads:[~2021-04-12 15:25 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-11 2:19 [PATCH][next] media: siano: Fix multiple out-of-bounds warnings in smscore_load_firmware_family2() Gustavo A. R. Silva
2021-03-26 16:30 ` Gustavo A. R. Silva
2021-04-12 15:00 ` Gustavo A. R. Silva [this message]
2021-05-11 15:18 ` Gustavo A. R. Silva
2021-05-11 15:47 ` Gustavo A. R. Silva
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=91073669-7a9b-9909-baa5-d4ddd814aa35@embeddedor.com \
--to=gustavo@embeddedor.com \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.