| Message ID | ZSRJfRdUXQOzagKr@work (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [next] media: venus: hfi_cmds: Replace one-element array with flex-array member and use __counted_by | expand |
On Mon, Oct 09, 2023 at 12:42:05PM -0600, Gustavo A. R. Silva wrote: > Array `data` in `struct hfi_sfr` is being used as a fake flexible array > at run-time: > > drivers/media/platform/qcom/venus/hfi_venus.c: > 1033 p = memchr(sfr->data, '\0', sfr->buf_size); > 1034 /* > 1035 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates > 1036 * that Venus is in the process of crashing. > 1037 */ > 1038 if (!p) > 1039 sfr->data[sfr->buf_size - 1] = '\0'; > 1040 > 1041 dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data); > > Fake flexible arrays are deprecated, and should be replaced by > flexible-array members. So, replace one-element array with a > flexible-array member in `struct hfi_sfr`. > > While there, also annotate array `data` with __counted_by() to prepare > for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for > array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > This results in no differences in binary output. Thanks for checking! > > This issue was found with the help of Coccinelle, and audited and fixed > manually. > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by: Kees Cook <keescook@chromium.org>
On (23/10/09 12:52), Kees Cook wrote: > On Mon, Oct 09, 2023 at 12:42:05PM -0600, Gustavo A. R. Silva wrote: > > Array `data` in `struct hfi_sfr` is being used as a fake flexible array > > at run-time: > > > > drivers/media/platform/qcom/venus/hfi_venus.c: > > 1033 p = memchr(sfr->data, '\0', sfr->buf_size); > > 1034 /* > > 1035 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates > > 1036 * that Venus is in the process of crashing. > > 1037 */ > > 1038 if (!p) > > 1039 sfr->data[sfr->buf_size - 1] = '\0'; > > 1040 > > 1041 dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data); > > > > Fake flexible arrays are deprecated, and should be replaced by > > flexible-array members. So, replace one-element array with a > > flexible-array member in `struct hfi_sfr`. > > > > While there, also annotate array `data` with __counted_by() to prepare > > for the coming implementation by GCC and Clang of the __counted_by > > attribute. Flexible array members annotated with __counted_by can have > > their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for > > array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > functions). > > > > This results in no differences in binary output. > > Thanks for checking! Sorry for shameless plug, a quick question: has any compiler implemented support for counted_by() at this point?
> Sorry for shameless plug, a quick question: has any compiler implemented > support for counted_by() at this point? > Not yet. And at least for GCC, it's expected to be released in v15. Thanks -- Gustavo
On (24/01/09 07:17), Gustavo A. R. Silva wrote: > > > Sorry for shameless plug, a quick question: has any compiler implemented > > support for counted_by() at this point? > > > > Not yet. And at least for GCC, it's expected to be released in v15. I see. Thank you. I got confused by include/linux/compiler_attributes.h comment, as I'm on clang-18 currently, seems that we need to bump min compilers version. Oh, and clang link 404-s on me. I'll send a quick patch, I guess.
On 1/9/24 07:28, Sergey Senozhatsky wrote: > On (24/01/09 07:17), Gustavo A. R. Silva wrote: >> >>> Sorry for shameless plug, a quick question: has any compiler implemented >>> support for counted_by() at this point? >>> >> >> Not yet. And at least for GCC, it's expected to be released in v15. > > I see. Thank you. > > I got confused by include/linux/compiler_attributes.h comment, as I'm on > clang-18 currently, seems that we need to bump min compilers version. Ah yes, compiler devs have been running into some issues, and they had to postpone the release of the attribute. > Oh, and clang link 404-s on me. I'll send a quick patch, I guess. > You're right, ick! -- Gustavo
diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h index dd9c5066442d..20acd412ee7b 100644 --- a/drivers/media/platform/qcom/venus/hfi_cmds.h +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h @@ -242,7 +242,7 @@ struct hfi_session_parse_sequence_header_pkt { struct hfi_sfr { u32 buf_size; - u8 data[1]; + u8 data[] __counted_by(buf_size); }; struct hfi_sys_test_ssr_pkt {
Array `data` in `struct hfi_sfr` is being used as a fake flexible array at run-time: drivers/media/platform/qcom/venus/hfi_venus.c: 1033 p = memchr(sfr->data, '\0', sfr->buf_size); 1034 /* 1035 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates 1036 * that Venus is in the process of crashing. 1037 */ 1038 if (!p) 1039 sfr->data[sfr->buf_size - 1] = '\0'; 1040 1041 dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data); Fake flexible arrays are deprecated, and should be replaced by flexible-array members. So, replace one-element array with a flexible-array member in `struct hfi_sfr`. While there, also annotate array `data` with __counted_by() to prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). This results in no differences in binary output. This issue was found with the help of Coccinelle, and audited and fixed manually. Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)