Re: [PATCH 1/2] media: venus: fix OOB read issue due to double read

[Vedang Nagar <quic_vnagar@quicinc.com>]

Hi Bryan,

On 1/17/2025 3:55 PM, Bryan O'Donoghue wrote:
> On 17/01/2025 08:39, Vedang Nagar wrote:
>> Below is the first read where dwords is being validated properly with the checks.
>> dwords = *rd_ptr >> 2;
>>
>> Whereas the same address is being read for the second time:
>> memcpy(pkt, rd_ptr, dwords << 2);
>>
>> For the second read the value is not validated which may get updated from the firmware
>> leading to incorrect memcpy into the packet and may lead to OOB read access while accessing
>> the packet.
> 
> So you are saying that pkt points to memory that the firmware and host can simultaneously access.
> 
> The question is - if the length value can change between one read and another read - how do you trust the _content_ of the packet ?
Original content of the packet is validated while reading the packet in hfi_process_msg_packet function.
Whereas the current change is just to validate the size of the packet to avoid the Out of bound read access.
> 
> Surely the right thing to do is to take a _copy_ of the entire frame and act on that frame exclusively on the host side ?
> 
> If I receive a frame, and read length X.
> 
> Then I need to re-read that frame because length may now by X+3.
> 
> This implies the _data_ in the frame has changed.
Yes, the _data_ in the frame has changed and will get rejected while parsing that data.
So I think it's okay to no read or copy the entire frame again.
> 
> What exactly is the valid lifetime of this data from HFI RX interrupt ?
There is no as such lifetime of the interrupt, but any rogue firmware can corrupt the data in the packet.

Regards,
Vedang Nagar
> 
> ---
> bod