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

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: <oxbpd3tfemwci6aiv5gs6rleg6lmsuabvvccqibbqddczjklpi@aln6hfloqizo>
Date: Thu, 7 Nov 2024 15:54:18 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Vikash Garodia <quic_vgarodia@...cinc.com>
Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, 
	Stanimir Varbanov <stanimir.k.varbanov@...il.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of
 bound access

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.

> 
> > 
> >>> +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
> > 

-- 
With best wishes
Dmitry