Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 29/08/2023 09:00, Vikash Garodia wrote:
> Hi Bryan,
> 
> On 8/14/2023 7:45 PM, Bryan O'Donoghue wrote:
>> On 14/08/2023 07:34, Vikash Garodia wrote:
>>>> We have two loops that check for up to 32 indexes per loop. Why not have a
>>>> capabilities index that can accommodate all 64 bits ?
>>> Max codecs supported can be 32, which is also a very high number. At max the
>>> hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is
>>> would not be needed.
>>>
>>
>> But the bug you are fixing here is an overflow where we have received a full
>> range 32 bit for each decode and encode.
>>
>> How is the right fix not to extend the storage to the maximum possible 2 x 32 ?
>> Or indeed why not constrain the input data to 32/2 for each encode/decode path ?
> At this point, we agree that there is very less or no possibility to have this
> as a real usecase i.e having 64 (or more than 32) codecs supported in video
> hardware. There seem to be no value add if we are extending the cap array from
> 32 to 64, as anything beyond 32 itself indicates rogue firmware. The idea here
> is to gracefully come out of such case when firmware is responding with such
> data payload.
> Again, lets think of constraining the data to 32/2. We have 2 32 bit masks for
> decoder and encoder. Malfunctioning firmware could still send payload with all
> bits enabled in those masks. Then the driver needs to add same check to avoid
> the memcpy in such case.
> 
>> The bug here is that we can copy two arrays of size X into one array of size X.
>>
>> Please consider expanding the size of the storage array to accommodate the full
>> size the protocol supports 2 x 32.
> I see this as an alternate implementation to existing handling. 64 index would
> never exist practically, so accommodating it only implies to store the data for
> invalid response and gracefully close the session.

What's the contractual definition of "this many bits per encoder and 
decoder" between firmware and APSS in that case ?

Where do we get the idea that 32/2 per encoder/decoder is valid but 32 
per encoder decoder is invalid ?

At this moment in time 16 encoder/decoder bits would be equally invalid.

I suggest the right answer is to buffer the protocol data unit - PDU 
maximum as an RX or constrain the maximum number of encoder/decoder bits 
based on HFI version.

ie.

- Either constrain on the PDU or
- Constrain on the known number of maximum bits per f/w version

---
bod