| Message ID | 20210311021947.GA129388@embeddedor (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] media: siano: Fix multiple out-of-bounds warnings in smscore_load_firmware_family2() | expand |
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 { >
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 { >>
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@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 { >>
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@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 { >>>
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 {
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(-)