Message ID | 20210511174547.GA33234@embeddedor (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RESEND,next] media: siano: Fix out-of-bounds warnings in smscore_load_firmware_family2() | expand |
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@chromium.org> > Signed-off-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > 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 { >
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@chromium.org> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> 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 { >>
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 {