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

  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(+)
  

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 ?

---
bod
  

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
  

On 11/08/2023 06:54, Vikash Garodia wrote:
> 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.

Then why favour the old cap report over the new ?
  

On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote:
> On 11/08/2023 06:54, Vikash Garodia wrote:
>> 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.
> 
> Then why favour the old cap report over the new ?

When the driver hits the case for OOB, thats when it knows that something has
gone wrong. Keeping old or new, both are invalid values in such case, nothing to
favor any value.

Regards,
Vikash
  

On 11/08/2023 09:51, Vikash Garodia wrote:
> 
> On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote:
>> On 11/08/2023 06:54, Vikash Garodia wrote:
>>> 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.
>>
>> Then why favour the old cap report over the new ?
> 
> When the driver hits the case for OOB, thats when it knows that something has
> gone wrong. Keeping old or new, both are invalid values in such case, nothing to
> favor any value.
> 
> Regards,
> Vikash

Is this hypothetical or a real bug you are actually working to mitigate ?

---
bod
  

On 8/11/2023 4:09 PM, Bryan O'Donoghue wrote:
> On 11/08/2023 09:51, Vikash Garodia wrote:
>>
>> On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote:
>>> On 11/08/2023 06:54, Vikash Garodia wrote:
>>>> 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.
>>>
>>> Then why favour the old cap report over the new ?
>>
>> When the driver hits the case for OOB, thats when it knows that something has
>> gone wrong. Keeping old or new, both are invalid values in such case, nothing to
>> favor any value.
>>
>> Regards,
>> Vikash
> 
> Is this hypothetical or a real bug you are actually working to mitigate ?

These are theoretical bugs, not reported during any video usecase so far. At the
same time, these are quite possible when the packets from firmware goes
different than expected.

> ---
> bod