linux-kernel - Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

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]

Message-ID: <a1713beb-e1bc-4118-ab58-b5d8e7fb3cbf@quicinc.com>
Date:   Fri, 11 Aug 2023 11:24:23 +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 3/4] venus: hfi: add checks to handle capabilities from
 firmware


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@...r.kernel.org
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@...cinc.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