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

[Vedang Nagar]

Hi Bryan,

On 2/16/2025 9:33 PM, Bryan O'Donoghue wrote:
> On 15/02/2025 17:19, Vedang Nagar wrote:
> > This series primarily adds check at relevant places in venus driver
> > where there are possible OOB accesses due to unexpected payload
> > from venus firmware. The patches describes the specific OOB possibility.
> >
> > Signed-off-by: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - Decompose sequence change event function.
> > - Fix repopulating the packet .with the first read during read_queue.
> > - Link to v1: https://lore.kernel.org/r/20250104-venus-security-fixes- 
> > v1-0-9d0dd4594cb4@xxxxxxxxxxx
> >
> > ---
> > Vedang Nagar (2):
> >        media: venus: fix OOB read issue due to double read
> >        media: venus: fix OOB access issue while reading sequence 
> > changed events
> >
> >   drivers/media/platform/qcom/venus/hfi_msgs.c  | 72 +++++++++++++++++ 
> > ++++++----
> >   drivers/media/platform/qcom/venus/hfi_venus.c |  1 +
> >   2 files changed, 63 insertions(+), 10 deletions(-)
> > ---
> > base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
> > change-id: 20241211-venus-security-fixes-50c22e2564d5
> >
> > Best regards,
>
> Could you please address the feedback I gave you / questions posited in 
> these two messages ?
>
> 4cfc1fe1-2fab-4256-9ce2-b4a0aad1069e@xxxxxxxxxx
>
> 0eab7323-ce86-40c7-9737-06eedcdf492d@xxxxxxxxxx
>
> 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.

[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.

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.

Please let me know if you have any other suggestions.

Regards,
Vedang Nagar
> ---
> bod