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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <21e7b893-2b84-f809-9775-f0c9c86c96a2@quicinc.com>
Date:   Tue, 29 Aug 2023 19:36:47 +0530
From:   Vikash Garodia <quic_vgarodia@...cinc.com>
To:     Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        <stanimir.k.varbanov@...il.com>, <agross@...nel.org>,
        <andersson@...nel.org>, <konrad.dybcio@...aro.org>,
        <mchehab@...nel.org>, <hans.verkuil@...co.com>,
        <tfiga@...omium.org>
CC:     <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of
 codecs within range


On 8/29/2023 5:29 PM, Bryan O'Donoghue wrote:
> 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

Let me simply ask this - What benefit we will be getting with above approaches
over the existing handling ?

Thanks,
Vikash
> ---
> bod
>