[v3,0/2] venus driver fixes for vulnerabilities due to unexpected firmware payload

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.

Changes in v3:
- Add check for validating the size instead of forcefully updating it (Bryan)
- Reduce duplication of code while handling sequence change event (Vikash)
- Update the inst->error for failure case instead of slienly breaking (Bryan)
- Link to v2: https://lore.kernel.org/lkml/20250215-venus-security-fixes-v2-0-cfc7e4b87168@quicinc.com/

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@quicinc.com

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
Vedang Nagar (2):
      media: venus: fix TOCTOU vulnerability when reading packets from shared memory
      media: venus: Fix OOB read due to missing payload bound check

 drivers/media/platform/qcom/venus/hfi_msgs.c  | 83 +++++++++++++++++++--------
 drivers/media/platform/qcom/venus/hfi_venus.c |  3 +
 2 files changed, 61 insertions(+), 25 deletions(-)
---
base-commit: b64b134942c8cf4801ea288b3fd38b509aedec21
change-id: 20250514-venus-fixes-8d93bccd9b9d

Best regards,

On 15/05/2025 13:11, Vikash Garodia wrote:
>> 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.

I don't believe you have identified a vulnerability here.

You read a length field, you check that length field against a MAX size.

Re-reading to see if the firmware wrote new bad data to the transmitted 
packet in-memory is not a fix before or after the memcpy() because the 
time you do that re-read is not fixed - locked wrt the freerunning firmware.

---
bod

On 5/15/2025 6:17 PM, Bryan O'Donoghue wrote:
> On 15/05/2025 13:11, Vikash Garodia wrote:
>>> 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.
> 
> I don't believe you have identified a vulnerability here.
> 
> You read a length field, you check that length field against a MAX size.
> 
> Re-reading to see if the firmware wrote new bad data to the transmitted packet
> in-memory is not a fix before or after the memcpy() because the time you do that
> re-read is not fixed - locked wrt the freerunning firmware.
It would be more meaningful if you can suggest the vulnerability you see with
the changes suggested i.e adding the check in local packet against the size read
from shared queue. Based on that we can see how to fix it, otherwise this
discussion in not leading to any conclusion.

Regards,
Vikash

On 15/05/2025 14:23, Vikash Garodia wrote:
>> Re-reading to see if the firmware wrote new bad data to the transmitted packet
>> in-memory is not a fix before or after the memcpy() because the time you do that
>> re-read is not fixed - locked wrt the freerunning firmware.
> It would be more meaningful if you can suggest the vulnerability you see with
> the changes suggested i.e adding the check in local packet against the size read
> from shared queue. Based on that we can see how to fix it, otherwise this
> discussion in not leading to any conclusion.

So to re-iterate.

TOCTOU is this

if (*ptr_val >> 2 >= MAX)
     return -EBAD;

memcpy(dst, src, *ptr_val >> 2);

Here a malicious actor can change *ptr_val between our check and our use.

not

data_value = *ptr_val >> 2;

if (data_value >= MAX)
     return -EBAD;

memcpy(dst, src, data_value);

Here the taking a copy of the value and subsequently relying on that 
value mitigates TOCTOU, because the value *ptr_val is latched - read 
into a local variable which cannot be manipulated from an outside agent 
i.e. venus firmware.

The example in the commit log is not a TOCTOU for that reason.

Adding an additional check _after_ the memcpy() seems silly to me because

data_value = *ptr_val >> 2;

if (data_value >= MAX)
     return -EBAD;

memcpy(dst, src, data_value);

// This statement could be false
if (data_value != *ptr_value >> 2)
     return -EBAD;

// while this subsequent statement is true
if (data_value != *ptr_value >> 2)
     return -EBAD;

And in any case this is a post-use sanity check not a mitigation for 
TOCTOU which we don't have.

---
bod

On 5/15/2025 11:21 PM, Bryan O'Donoghue wrote:
> On 15/05/2025 14:23, Vikash Garodia wrote:
>>> Re-reading to see if the firmware wrote new bad data to the transmitted packet
>>> in-memory is not a fix before or after the memcpy() because the time you do that
>>> re-read is not fixed - locked wrt the freerunning firmware.
>> It would be more meaningful if you can suggest the vulnerability you see with
>> the changes suggested i.e adding the check in local packet against the size read
>> from shared queue. Based on that we can see how to fix it, otherwise this
>> discussion in not leading to any conclusion.
> 
> So to re-iterate.
> 
> TOCTOU is this
> 
> if (*ptr_val >> 2 >= MAX)
>     return -EBAD;
> 
> memcpy(dst, src, *ptr_val >> 2);
> 
> Here a malicious actor can change *ptr_val between our check and our use.
> 
> not
> 
> data_value = *ptr_val >> 2;
> 
> if (data_value >= MAX)
>     return -EBAD;
> 
> memcpy(dst, src, data_value);
> 
> Here the taking a copy of the value and subsequently relying on that value
> mitigates TOCTOU, because the value *ptr_val is latched - read into a local
> variable which cannot be manipulated from an outside agent i.e. venus firmware.
> 
> The example in the commit log is not a TOCTOU for that reason.
> 
> Adding an additional check _after_ the memcpy() seems silly to me because
> 
> data_value = *ptr_val >> 2;
> 
> if (data_value >= MAX)
>     return -EBAD;
> 
> memcpy(dst, src, data_value);
> 
> // This statement could be false
> if (data_value != *ptr_value >> 2)
>     return -EBAD;
> 
> // while this subsequent statement is true
> if (data_value != *ptr_value >> 2)
>     return -EBAD;
> 
Check the pseudo code which i proposed earlier in this conversation [1]. It does
not rely on ptr_val at all to check the sanity after memcpy.

[1] https://lore.kernel.org/all/0c50c24a-35fa-acfb-a807-b4ed5394506b@quicinc.com/
> And in any case this is a post-use sanity check not a mitigation for TOCTOU
> which we don't have.
> 
> ---
> bod

On 15/05/2025 19:25, Vikash Garodia wrote:
> Check the pseudo code which i proposed earlier in this conversation [1]. It does
> not rely on ptr_val at all to check the sanity after memcpy.
> 
> [1]https://lore.kernel.org/all/0c50c24a-35fa-acfb-a807- 
> b4ed5394506b@quicinc.com/

Understood.

Another version of this patch to check after the memcpy() for 
verification purposes might be correct but, IMO there's no scope for a 
TOCTOU based modification here.

---
bod