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

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

On 3/4/2025 7:15 PM, Bryan O'Donoghue wrote:
> On 15/02/2025 17:19, Vedang Nagar wrote:
>> During message queue read, the address is being read twice
>> from the shared memory. The first read is validated against
>> the size of the packet, however the second read is not
>> being validated. Therefore, it's possible for firmware to
>> modify the value to a bigger invalid value which can lead
>> to OOB read access issue while reading the packet.
>> Added fix to reupdate the size of the packet which was
>> read for the first time.
>>
>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index
>> f9437b6412b91c2483670a2b11f4fd43f3206404..c124db8ac79d18f32289a690ee82145dc93daee6 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -298,6 +298,7 @@ static int venus_read_queue(struct venus_hfi_device
>> *hdev,
>>               memcpy(pkt, rd_ptr, len);
>>               memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
>>           }
>> +        *(u32 *)pkt = dwords << 2;
>>       } else {
>>           /* bad packet received, dropping */
>>           new_rd_idx = qhdr->write_idx;
>>
> 
> This is confusing - where is the read
> 
> Your previous code
> 
> https://lore.kernel.org/lkml/20250104-venus-security-fixes-v1-1-9d0dd4594cb4@quicinc.com/
> 
> memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
> 
> V1 then would have been:
> 
> if (new_rd_idx < qsize) {
>     memcpy(pkt, rd_ptr, dwords << 2);
> } else {
>     size_t len;
> 
>     new_rd_idx -= qsize;
>     len = (dwords - new_rd_idx) << 2;
>     memcpy(pkt, rd_ptr, len);
>     memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
> }
> 
> + memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
> 
> V2 proposed:
> 
> if (new_rd_idx < qsize) {
>     memcpy(pkt, rd_ptr, dwords << 2);
> } else {
>     size_t len;
> 
>     new_rd_idx -= qsize;
>     len = (dwords - new_rd_idx) << 2;
>     memcpy(pkt, rd_ptr, len);
>     memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
> }
> 
> + *(u32 *)pkt = dwords << 2;
> 
> My comment wasn't about use of memcpy() it was about why we are doing this.
> 
> For example if new_rd_idx < qsize is true then we literally do
> 
> a) memcpy(pkt, rd_ptr, dwords << 2);
> b) *(u32 *)pkt = dwords << 2;
> 
> and the question is why ? That is an unambiguous cast of pkt to the value
> of dwords << 2;
> 
> What is the scope of how the data can change from a to b ?
> 
> And why is the data considered potentially invalid @ the memcpy() but valid
> subsequent the cast ?
> 
The concern here is not about the value of dword which will be fixed once
we read the *rd_ptr first time and validate it.

The concern here is, with the data contents at rd_ptr which could be
tempered with after validation, because the memory is shared by firmware

So the real problem here is:
- driver read *rd_ptr, extract dwords = *rd_ptr >> 2, and validate it, lets
say this value is 10
- then copy dwords << 2 bytes from rd_ptr to a kernel buffer (pkt).
- But: the first 4 bytes at rd_ptr (which represent the size field again)
could have changed in the meantime — now saying 40 instead of the original 10.

security concern here is:
Even though the outer dwords is correct, the copied content might claim to
be bigger than what was validated — potentially tricking the rest of the
system into processing garbage or running past buffer bounds.

This is not a TOCTOU on the dwords, but a TOCTOU on the contents at rd_ptr,
where the firmware changes the actual data after size is checked, but
before it's copied.

> Assuming rd_ptr contains the length of dwords << 2 to begin with in the
> first 4 bytes - why is it necessary to make _really_ _really_ sure by
> restuffing the data ?
> 
> For example if *(u32 *)rd_ptr != dwords << 2 - why shouldn't we just throw
> the whole frame away as containing junk data ?

Agree with returning error during mismatch instead of forcefully
overwriting the content.

Thank,
Dikshita
> 
> ---
> bod
>