Re: [PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Bryan O'Donoghue]

On 17/01/2025 08:39, Vedang Nagar wrote:
> Hi Bryan,
>
> On 1/6/2025 5:36 AM, Bryan O'Donoghue wrote:
> > On 04/01/2025 05:41, Vedang Nagar wrote:
> > > num_properties_changed is being read from the message queue but is
> > > not validated. Value can be corrupted from the firmware leading to
> > > OOB read access issues. Add fix to read the size of the packets as
> > > well and crosscheck before reading from the packet.
> > >
> > > Signed-off-by: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
> > Please see Vikash's series on this.
> >
> > https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@xxxxxxxxxxx/
> >
> > it seems to have exactly the same patch title ?
> >
> > Is this patch supposed to be a follow-up to that patch ?
> >
> > https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@xxxxxxxxxxx/
> >
> > Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here.
> No, this is a different series having OOB fixes similar to ones posted by Vikash.

OK, please use a different patch title.

I understand the motive to repeat the patch title but, its confusing. If 
you added some text to make the OOB more specific then it would be 
possible to differentiate between.

"fix OOB access issue while reading sequence changed events 'in some 
location' || 'on some path'"


> > On the switch statement I'd have two comments.
> >
> > #1 is everything really a " -= sizeof(u32)" ?
> Yes, it's everytime " -= sizeof(u32) " since the first the first word read is ptype of size u32
> > #2 if so then this ought to be factored out into a function
> >     => functional decomposition
> Sure, will fix this with decomposition into functions.

Is firmware sending a change event or updating a packet already in memory ?

What is the nature of the change event and how do you guarantee the 
second read is valid when the first read can be considered invalid ?

i.e.

- Read - derive read value X.
- Do some stuff.
- Read - again to make sure length value is still X.
- Do all sorts of other processing.

At which point is the sequence considered complete and the data 
considered "locked" and valid ?

What happens if you get a subsequent change event once this procedure 
has completed ?

---
bod