Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Vikash Garodia <quic_vgarodia@quicinc.com>]

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