Re: [PATCH RESEND][next] media: siano: Fix out-of-bounds warnings in smscore_load_firmware_family2()
From: Gustavo A. R. Silva
Date: Mon Jun 07 2021 - 15:03:40 EST
Hi all,
As I haven't received any response for this patch in almost three months[1] since I first
sent it out, I'm taking this in my -next[2] branch for v5.14.
Thanks
--
Gustavo
[1] https://lore.kernel.org/linux-hardening/20210311021947.GA129388@embeddedor/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp
On 6/1/21 17:12, Gustavo A. R. Silva wrote:
> 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 {
>>