Re: [PATCH][next] media: siano: Fix multiple out-of-bounds warnings in smscore_load_firmware_family2()
From: Gustavo A. R. Silva
Date: Tue May 11 2021 - 11:47:26 EST
By the way, we are about to be able to globally enable -Warray-bounds and,
these are the last out-of-bounds warnings in linux-next.
Thanks
--
Gustavo
On 5/11/21 10:18, Gustavo A. R. Silva wrote:
> Hi all,
>
> Friendly ping (second one): 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@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 {
>>>