Re: [PATCH v3 1/2] media: venus: fix TOCTOU vulnerability when reading packets from shared memory

[Vikash Garodia <quic_vgarodia@quicinc.com>]

On 5/15/2025 3:58 PM, Bryan O'Donoghue wrote:
> On 15/05/2025 10:56, Vikash Garodia wrote:
>> memcpy(hfi_dev->pkt_buf, rd_ptr from shared queue, dwords..)
>>
>> pkt_hdr = (struct hfi_pkt_hdr *) (hfi_dev->pkt_buf);
>>
>> if ((pkt_hdr->size >> 2) != dwords)
>>      return -EINVAL;
> 
> Yeah it would be better wrt the commit log.
> 
> But does it really give additional data-confidence - I don't believe it does.
> 
> => The firmware can update the pkt header after our subsequent-to-memcpy() check.
How will that matter if the queue is updated after memcpy to local packet ? All
processing of data would be from local packet.

> 
> Again this is a data-lifetime expectation question.
> 
> You validate the received data against a maximum size reading to a buffer you
> know the size of - and do it once.
> 
> The firmware might corrupt that data in-between but that is not catastrophic for
> the APSS which has a buffer of a known size containing potential bad data.
There is no way to authenticate the content of payload. We are trying to avoid
vulnerability by ensuring OOB does not happen by validating the size _alone_. Do
you see rest of the data in payload can lead to any kind of vulnerability ?

> 
> Fine - and additional check after the mempcy() only imparts verisimilitude -
> only validates our data at the time of the check.
> 
> my-linear-uninterrupted-context:
> 
> memcpy();
> 
> if(*rd_ptr >> 2 > len) <- doesn't branch
>     return -EBAD
> 
> if(*rd_ptr >> 2 > len) <- does branch firmware went nuts
>     return -EBAD
> 
> Superficially you might say this addresses the problem
> 
> if (*rd_ptr > MAX)
>     return -EBAD;
> 
> memcpy();
> 
> if (*rd_ptr > MAX)
>     return -EBAD;
> 
> But what if the "malicious" firmware only updated the data in the packet, not
> the length - or another field we are not checking ?
That does not cause any vulnerability. You can check and suggest if you see a
vulnerability when the data outside length is an issue w.r.t vulnerability.

> 
> As I say if this can happen
> 
> 
> if (*rd_ptr > MAX)
>     return -EBAD;
> 
> memcpy();
> 
> if (*rd_ptr > MAX)  // good
>     return -EBAD;
> 
> 
> if (*rd_ptr > MAX) //bad
>     return -EBAD;
> 
> then this can happen
> 
> if (*rd_ptr > MAX)
>     return -EBAD;
> 
> memcpy();
> 
> if (*rd_ptr > MAX) // good
>     return -EBAD;
> 
> 
> if (*rd_ptr > MAX) //good
>     return -EBAD;
> 
> if (*rd_ptr > MAX) //bad
>     return -EBAD;
> 
> We need to have a crisp and clear definition of the data-lifetime. Since we
> don't have a checksum element in the header the only check that makes sense is
> to validate the buffer size
> 
> data_len = *ptr_val >> 2;
> if (data_len > max)
>     return BAD;
> 
> Using the data_len in memcpy if the *ptr_val can change is _NOT_ TOCTOU
> 
> This is TOCTOU
> 
> if (*ptr_val > max)
>     return EBAD;
> 
> memcpy(dst, src, *ptr_val);
> 
> Because I validated the content of the pointer and then I relied on the data
> that pointer pointed to, which might have changed.
Yes, precisely for that, memcpy() does not rely on ptr_val. The one you are
referring as data_len is same as dword.

Regards,
Vikash
> 
> TBH I think the entire premise of this patch is bogus.
> 
> ---
> bod