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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Aug 2023 13:30:56 +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

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.

Thanks,
Vikash