On 8/10/2023 5:01 PM, Bryan O'Donoghue wrote:
On 10/08/2023 03:25, Vikash Garodia wrote:
The hfi parser, parses the capabilities received from venus firmware and copies them to core capabilities. Consider below api, for example, fill_caps - In this api, caps in core structure gets updated with the number of capabilities received in firmware data payload. If the same api is called multiple times, there is a possibility of copying beyond the max allocated size in core caps. Similar possibilities in fill_raw_fmts and fill_profile_level functions.
Cc: stable@vger.kernel.org Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser") Signed-off-by: Vikash Garodia quic_vgarodia@quicinc.com
drivers/media/platform/qcom/venus/hfi_parser.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c index 6cf74b2..9d6ba22 100644 --- a/drivers/media/platform/qcom/venus/hfi_parser.c +++ b/drivers/media/platform/qcom/venus/hfi_parser.c @@ -86,6 +86,9 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data, { const struct hfi_profile_level *pl = data; + if (cap->num_pl + num >= HFI_MAX_PROFILE_COUNT) + return;
memcpy(&cap->pl[cap->num_pl], pl, num * sizeof(*pl)); cap->num_pl += num; }
Why append and discard though ?
Couldn't we reset/reinitalise the relevant indexes in hfi_sys_init_done() ?
Can subsequent notifications from the firmware give a new capability set ? Presumably not.
IMO though instead of throwing away the new data, we should throw away the old data, no ?
The case is all about rogue firmware. If there is a need to fill the same cap again, that itself indicates that the payload from firmware is not correct. In such cases, the old as well as new cap data are not reliable. Though the authenticity of the data cannot be ensured, the check would avoid any OOB during such rogue firmware case.
Regards, Vikash
bod