Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/7/2024 7:24 PM, Dmitry Baryshkov wrote:
> On Thu, Nov 07, 2024 at 07:05:15PM +0530, Vikash Garodia wrote:
>>
>> On 11/7/2024 6:52 PM, Dmitry Baryshkov wrote:
>>> On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote:
>>>>
>>>> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote:
>>>>> On 07/11/2024 10:41, Dmitry Baryshkov wrote:
>>>>>>> init_codecs() parses the payload received from firmware and . I don't think we
>>>>>>> can control this part when we have something like this from a malicious firmware
>>>>>>> payload
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>>>> ...
>>>>>>> Limiting it to second iteration would restrict the functionality when property
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
>>>>>> If you can have a malicious firmware (which is owned and signed by
>>>>>> Qualcomm / OEM), then you have to be careful and skip duplicates. So
>>>>>> instead of just adding new cap to core->caps, you have to go through
>>>>>> that array, check that you are not adding a duplicate (and report a
>>>>>> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
>>>>>>
>>>>>> Just ignoring the "extra" entries is not enough.
>>>> Thinking of something like this
>>>>
>>>> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
>>>>     if (core->codecs_count >= MAX_CODEC_NUM)
>>>>         return;
>>>>     cap = &caps[core->codecs_count++];
>>>>     if (cap->codec == BIT(bit)) --> each code would have unique bitfield
>>>>         return;
>>>
>>> This won't work and it's pretty obvious why.
>> Could you please elaborate what would break in above logic ?
> 
> After the "cap=&caps[core->codecs_count++]" line 'cap' will point to the
> new entry, which should not contain valid data.
> 
> Instead, when processing new 'bit' you should loop over the existing
> caps and check that there is no match. And only if there is no match
> the code should be allocating new entry, checking that codecs_count
> doesn't overflow, etc.
Got it.

Regards,
Vikash
>>
>>>
>>>>> +1
>>>>>
>>>>> This is a more rational argument. If you get a second message, you should surely
>>>>> reinit the whole array i.e. update the array with the new list, as opposed to
>>>>> throwing away the second message because it over-indexes your local storage..
>>>> That would be incorrect to overwrite the array with new list, whenever new
>>>> payload is received.
>>>
>>> I'd say, don't overwrite the array. Instead the driver should extend it
>>> with the new information.
>> That is exactly the existing patch is currently doing.
> 
> _new_ information, not a copy of the existing information.
> 
>>
>> Regards,
>> Vikash
>>>
>>>>
>>>> Regards,
>>>> Vikash
>>>
>