Re: [PATCH v2 0/2] venus driver fixes to avoid possible OOB read access

[Bryan O'Donoghue]

On 03/03/2025 13:12, Vikash Garodia wrote:
> On 3/2/2025 9:26 PM, Bryan O'Donoghue wrote:
> > On 02/03/2025 11:58, Vedang Nagar wrote:
> > > > The basic question : what is the lifetime of the data from RX interrupt to
> > > > consumption by another system agent, DSP, userspace, whatever ?
> > > As mentioned in [1], With the regular firmware, after RX interrupt the data
> > > can be considered as valid until next interrupt is raised, but with the rouge
> > > firmware, data can get invalid during the second read and our intention is to
> > > avoid out of bound access read because of such issues.
> >
> > This is definitely the part I don't compute.
> >
> > 1. RX interrupt
> > 2. Frame#0 Some amount of time data is always valid
> This is not correct. Its not the amount of time which determines the validity of
> the data, its the possibility of rogue firmware which, if incase, puts up the
> date in shared queue, would always be invalid, irrespective of time.
>
> > 3. RX interrupt - new data
> > 4. Frame#1 new data delivered into a buffer
> >
> > Are you describing a case between RX interrupts 1-3 or a case after 1-4?
> >
> > Why do we need to write code for rouge firmware anyway ?
> It is a way to prevent any possibility of OOB, similar to how any API does check
> for validity of any arguments passed to it, prior to processing.
> > And the real question - if the data can be invalidated in the 1-3 window above
> > when is the safe time to snapshot that data ?
> >
> > We seem to have alot of submissions to deal with 'rouge' firmware without I
> > think properly describing the problem of the _expected_ data lifetime.
> >
> > So
> >
> > a) What is the expected data lifetime of an RX buffer between one
> >     RX IRQ and the next ?
> >     I hope the answer to this is - APSS owns the buffer.
> >     This is BTW usually the case in these types of asymmetric setups
> >     with a flag or some other kind of semaphore that indicates which
> >     side of the data-exchange owns the buffer.
> >
> > b) In this rouge - buggy - firmware case what is the scope of the
> >     potential race condition ?
> >
> >     What I'd really like to know here is why we have to seemingly
> >     memcpy() again and again in seemingly incongrous and not
> >     immediately obvious places in the code.
> >
> >     Would we not be better advised to do a memcpy() of the entire
> >     RX frame in the RX IRQ handler path if as you appear to me
> >     suggesting - the firmware can "race" with the APSS
> >     i.e. the data-buffer ownership flag either doesn't work
> >     or isn't respected by one side in the data-exchange.
> >
> > Can we please have a detailed description of the race condition here ?
> Below is the report which the reporter reported leading to OOB, let me know if
> you are unable to deduce the trail leading to OOB here.
>
> OOB read issue is in function event_seq_changed, please reference below code
> snippet:
>
> Buggy code snippet:
>
> static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>          struct hfi_msg_event_notify_pkt *pkt)
> ...
> num_properties_changed = pkt->event_data2; //num_properties_changed is from
> message and is not validated.
> ...
> data_ptr = (u8 *)&pkt->ext_event_data[0];
> do {
>   ptype = *((u32 *)data_ptr);
>   switch (ptype) {
>   case HFI_PROPERTY_PARAM_FRAME_SIZE:
>    data_ptr += sizeof(u32);
>    frame_sz = (struct hfi_framesize *)data_ptr;
>    event.width = frame_sz->width;
> ...
>   }
>   num_properties_changed--;
> } while (num_properties_changed > 0);
> ```
> There is no validation against `num_properties_changed = pkt->event_data2`, so
> OOB read occurs.
> > I don't doubt the new memcpy() makes sense to you but without this detailed
> > understanding of the underlying problem its virtually impossible to debate the
> > appropriate remediation - perhaps this patch you've submitted - or some other
> > solution.
> >
> > Sorry to dig into my trench here but, way more detail is needed.
> >
> > > [1]: https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2-
> > > b4a0aad1069e@xxxxxxxxxx/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
> > > > Why is it in this small specific window that the data can change but not
> > > > later ? What is the mechanism the data can change and how do the changes you
> > > > propose here address the data lifetime problem ?
> > > Currently this issue has been discovered by external researchers at this
> > > point, but if any such OOB issue is discovered at later point as well then we
> > > shall fix them as well.
> >
> > Right but, I'm looking for a detailed description of the problem.
> >
> > Can you describe from RX interrupt again what the expected data lifetime of the
> > RX frame is, which I hope we agree is until the next RX interrupt associated
> > with a given buffer with an ownership flag shared between firmware and APSS -
> > and then under what circumstances that "software contract" is being violated.
> >
> > > Also, with rougue firmware we cannot fix the data lifetime problem in my
> > > opinion, but atleast we can fix the out of bound issues.
> > > > Without that context, I don't believe it is really possible to validate an
> > > > additional memcpy() here and there in the code as fixing anything.
> > > There is no additional memcpy() now in the v2 patch, but as part of the fix,
> > > we are just trying to retain the length of the packet which was being read in
> > > the first memcpy() to avoid the OOB read access.
> >
> > I can't make a suggestion because - personally speaking I still don't quite
> > understand the data-race you are describing.
> Go through the reports from the reporter, it was quite evident in leading upto
> OOB case.
> Putting up the sequence for you to go over the interrupt handling and message
> queue parsing of the packets from firmware
> 1.
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_venus.c#L1082
> 2.
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L816
> 3. event handling (this particular case)
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L658
> 4.
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L22
>
> the "struct hfi_msg_event_notify_pkt *pkt" pkt here is having the data read from
> shared queue.
>
> > I get that you say the firmware is breaking the contract but, without more
> > detail on _how_ it breaks that contract I don't think it's really possible to
> > validate your fix here, fixes anything.
> >
> > ---
> > bod
>
> Regards,
> Vikash

I'll go through all of these links given here, thanks.

Whatever the result of the review, this detail needs to go into the 
commit log so that a reviewer can reasonably read the problem 
description and evaluate against submitted code as a fix.

---
bod