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 <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
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 {
--
2.27.0
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 <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> 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 {
>
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 <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>> ---
>> 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 {
>>
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 <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>> ---
>> 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 {
>>
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 <[email protected]>
>>> Signed-off-by: Kees Cook <[email protected]>
>>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>>> ---
>>> 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 {
>>>