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

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

[Dikshita Agarwal]

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,
-- 
Dikshita Agarwal <quic_dikshita@quicinc.com>

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

[Dikshita Agarwal]

From: Vedang Nagar <quic_vnagar@quicinc.com>

Currently, Time-Of-Check to Time-Of-Use (TOCTOU) issue happens when
handling packets from firmware via shared memory.

The problematic code pattern:

u32 dwords = *rd_ptr >> 2;
if (!dwords || (dwords << 2) >  IFACEQ_VAR_HUGE_PKT_SIZE))
   return -EINVAL;

memcpy(pkt, rd_ptr, dwords << 2);

Here, *rd_ptr is used to determine the size of the packet and is
validated. However, since rd_ptr points to firmware-controlled memory,
the firmware could change the contents (e.g., embedded header fields
like pkt->hdr.size) after the size was validated but before or during
the memcpy() call.

This opens up a race window where a malicious or buggy firmware could
inject inconsistent or malicious data, potentially leading to
information leaks, driver crashes, or undefined behavior.

Fix this by rechecking the packet size field from shared memory
immediately before the memcpy() to ensure it has not beenn altered.

Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
Co-developed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index b5f2ea8799507f9b83f1529e70061ea89a9cc5c8..163c8d16530bc44a84b2b21076e6189d476fe360 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -295,6 +295,9 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
 	new_rd_idx = rd_idx + dwords;
 	if (((dwords << 2) <= IFACEQ_VAR_HUGE_PKT_SIZE) && rd_idx <= qsize) {
 		if (new_rd_idx < qsize) {
+			if ((*rd_ptr >> 2) != dwords)
+				return -EINVAL;
+
 			memcpy(pkt, rd_ptr, dwords << 2);
 		} else {
 			size_t len;

-- 
2.34.1

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

[Bryan O'Donoghue]

On 14/05/2025 14:38, Dikshita Agarwal wrote:
> From: Vedang Nagar <quic_vnagar@quicinc.com>
> 
> Currently, Time-Of-Check to Time-Of-Use (TOCTOU) issue happens when
> handling packets from firmware via shared memory.
> 
> The problematic code pattern:
> 
> u32 dwords = *rd_ptr >> 2;
> if (!dwords || (dwords << 2) >  IFACEQ_VAR_HUGE_PKT_SIZE))
>     return -EINVAL;
> 
> memcpy(pkt, rd_ptr, dwords << 2);
> 
> Here, *rd_ptr is used to determine the size of the packet and is
> validated. However, since rd_ptr points to firmware-controlled memory,
> the firmware could change the contents (e.g., embedded header fields
> like pkt->hdr.size) after the size was validated but before or during
> the memcpy() call.
> 
> This opens up a race window where a malicious or buggy firmware could
> inject inconsistent or malicious data, potentially leading to
> information leaks, driver crashes, or undefined behavior.
> 
> Fix this by rechecking the packet size field from shared memory
> immediately before the memcpy() to ensure it has not beenn altered.
> 
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> Co-developed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index b5f2ea8799507f9b83f1529e70061ea89a9cc5c8..163c8d16530bc44a84b2b21076e6189d476fe360 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -295,6 +295,9 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
>   	new_rd_idx = rd_idx + dwords;
>   	if (((dwords << 2) <= IFACEQ_VAR_HUGE_PKT_SIZE) && rd_idx <= qsize) {
>   		if (new_rd_idx < qsize) {
> +			if ((*rd_ptr >> 2) != dwords)
> +				return -EINVAL;
> +
>   			memcpy(pkt, rd_ptr, dwords << 2);
>   		} else {
>   			size_t len;
> 

Here's how this code fragment looks after the change, I'll add two "}" 
for readability and annotate

dwords = *rd_ptr >> 2; // read the value here
if (!dwords)
        return -EINVAL;

new_rd_idx = rd_idx + dwords;

// validate the size against a maximum value
// this step is correct
if (((dwords << 2) <= IFACEQ_VAR_HUGE_PKT_SIZE) && rd_idx <= qsize) {
         if (new_rd_idx < qsize) {
                 // Re-read the value because firmware
                 // might have changed the value
                 if ((*rd_ptr >> 2) != dwords)
                         return -EINVAL;

                 // now trust dwords
                 memcpy(pkt, rd_ptr, dwords << 2);
         }
}

But this is all wrong.

There is no atomicity on the APSS side between the first verification of 
dwords size and the mempcpy() the commit log itself shows that the 
firmware is free-running with respect to the instruction pipeline of the 
APSS, it is an AMP problem.

Adding another check of the dwords side right before the memcpy() 
doesn't address the problem which the commit log describes as the 
firmware updating the length field of a header in shared memory.

There are perhaps 10 assembly instructions between the first check and 
the procedure prologue of the memcpy();

Adding another length check right before the memcpy() simply reduces the 
number of CPU instructions - the effective window that the firmware can 
update that header still.

if ((*rd_ptr >> 2) != dwords) // conditional branch operation

memcpy(pkt, rd_ptr, dwords << 2);

Begins with a procedure prologue - setting up the call stack - and then 
is a series of fetch/stores to copy data from here to there

The memcpy() itself by its nature it not atomic on the front-side-bus of 
the APSS to shared DRAM with the firmware.

On a CPU and SoC architecture level this fix just doesn't work.

To be honest we are already doing the right thing in this routine.

1. Reading the value from the packet header.
2. Validating the given size against the maximum size
3. Rejecting the memcpy() if the given size _at_the_time_we_read_ is too
    large.

The alternative to guarantee would be something like

asm("bus master asserts bus lock to PAGE/PAGES involved");
dwords = *rd_ptr;
if (dwords > MAX_VALUE)
     return -EFIRMWARE_BUG;
memcpy(dst, src, dwords >> 2);

asm("bus master unlocks memory");

Lets say we make the change proposed in this patch, here is how it breaks:

if ((*rd_ptr >> 2) != dwords)
     return -EINVAL;

// now trust dwords
memcpy(pkt, rd_ptr, dwords << 2);


objdump 
qlt-kernel/build/x1e80100-crd_qlt_integration/drivers/media/platform/qcom/venus/venus-core.o 
--disassemble=venus_read_queue.isra.0

5c48:	540000c9 	b.ls	5c60 <venus_read_queue.isra.0+0x110>  // b.plast
5c4c:	2a0303e2 	mov	w2, w3
5c50:	aa0703e0 	mov	x0, x7
5c54:	94000000 	bl	0 <memcpy>
5c58:	52800000 	mov	w0, #0x0

Your conditional jump is @ 0x5c48 your call to memcpy @ 0x5c54

Between 0x5c48 and 0x5c54 the firmware can update the value _again_
Indeed the firmware can update the value up until the time we complete 
reading the bit of the pkt header in memcpy() so an additional few 
instructions for sure.

You could make some type of argument to re-verify the content of the pkt 
_after_ the memcpy()

But the only verification that makes any sense _before_ the memcpy() is 
to verify the length at the point you _read_ - subsequent to the 
latching operation - were we fetch the length value from DRAM into our 
CPU cache, operating stack and/or local registers.

Once that data is fetched within the cache/stack/registers of the 
CPU/APSS that is the relevant value.

For the fix you have here to work you need this

5c48:   MAGICOP         memorybuslock
5c48:	540000c9 	b.ls	5c60 <venus_read_queue.isra.0+0x110>  // b.plast
5c4c:	2a0303e2 	mov	w2, w3
5c50:	aa0703e0 	mov	x0, x7
5c54:	94000000 	bl	0 <memcpy>
5c58:	52800000 	mov	w0, #0x0
5c5c:   MAGICUNOP       memorybusunlock

Because the firmware is free-running - with respect to the instruction 
pipeline of the above assembly.

If you really want to verify the data is still valid - it should be done 
_after_ the memcpy();

But even then I'd say to you, why verify _after_ the memcpy() - and what 
happens on the instruction directly _after_ the verification - is the 
data considered more valid now ?

i.e. this:

memcpy(pkt, rd_ptr, dwords << 2);

if ((*rd_ptr >> 2) != dwords)
     return -EINVAL;

doesn't have the above described architectural race condition but it 
doesn't make the data any more trustworthy - because it doesn't have 
atomicity

memcpy(pkt, rd_ptr, dwords << 2);

if ((*rd_ptr >> 2) != dwords)
     return -EINVAL;

dev_info(dev, "The value of *rd_ptr %lu!=%lu can be different now\n",
          *rd_ptr >> 2, dwords);

Sorry this patch just can't work. It's a very hard NAK from me.

---
bod

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

[Vikash Garodia]

On 5/15/2025 2:47 PM, Bryan O'Donoghue wrote:
> On 14/05/2025 14:38, Dikshita Agarwal wrote:
>> From: Vedang Nagar <quic_vnagar@quicinc.com>
>>
>> Currently, Time-Of-Check to Time-Of-Use (TOCTOU) issue happens when
>> handling packets from firmware via shared memory.
>>
>> The problematic code pattern:
>>
>> u32 dwords = *rd_ptr >> 2;
>> if (!dwords || (dwords << 2) >  IFACEQ_VAR_HUGE_PKT_SIZE))
>>     return -EINVAL;
>>
>> memcpy(pkt, rd_ptr, dwords << 2);
>>
>> Here, *rd_ptr is used to determine the size of the packet and is
>> validated. However, since rd_ptr points to firmware-controlled memory,
>> the firmware could change the contents (e.g., embedded header fields
>> like pkt->hdr.size) after the size was validated but before or during
>> the memcpy() call.
>>
>> This opens up a race window where a malicious or buggy firmware could
>> inject inconsistent or malicious data, potentially leading to
>> information leaks, driver crashes, or undefined behavior.
>>
>> Fix this by rechecking the packet size field from shared memory
>> immediately before the memcpy() to ensure it has not beenn altered.
>>
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
>> Co-developed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index
>> b5f2ea8799507f9b83f1529e70061ea89a9cc5c8..163c8d16530bc44a84b2b21076e6189d476fe360 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -295,6 +295,9 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
>>       new_rd_idx = rd_idx + dwords;
>>       if (((dwords << 2) <= IFACEQ_VAR_HUGE_PKT_SIZE) && rd_idx <= qsize) {
>>           if (new_rd_idx < qsize) {
>> +            if ((*rd_ptr >> 2) != dwords)
>> +                return -EINVAL;
>> +
>>               memcpy(pkt, rd_ptr, dwords << 2);
>>           } else {
>>               size_t len;
>>
> 
> Here's how this code fragment looks after the change, I'll add two "}" for
> readability and annotate
> 
> dwords = *rd_ptr >> 2; // read the value here
> if (!dwords)
>        return -EINVAL;
> 
> new_rd_idx = rd_idx + dwords;
> 
> // validate the size against a maximum value
> // this step is correct
> if (((dwords << 2) <= IFACEQ_VAR_HUGE_PKT_SIZE) && rd_idx <= qsize) {
>         if (new_rd_idx < qsize) {
>                 // Re-read the value because firmware
>                 // might have changed the value
>                 if ((*rd_ptr >> 2) != dwords)
>                         return -EINVAL;
> 
>                 // now trust dwords
>                 memcpy(pkt, rd_ptr, dwords << 2);
>         }
> }
> 
> But this is all wrong.
> 
> There is no atomicity on the APSS side between the first verification of dwords
> size and the mempcpy() the commit log itself shows that the firmware is
> free-running with respect to the instruction pipeline of the APSS, it is an AMP
> problem.
> 
> Adding another check of the dwords side right before the memcpy() doesn't
> address the problem which the commit log describes as the firmware updating the
> length field of a header in shared memory.
> 
> There are perhaps 10 assembly instructions between the first check and the
> procedure prologue of the memcpy();
> 
> Adding another length check right before the memcpy() simply reduces the number
> of CPU instructions - the effective window that the firmware can update that
> header still.
> 
> if ((*rd_ptr >> 2) != dwords) // conditional branch operation
> 
> memcpy(pkt, rd_ptr, dwords << 2);
> 
> Begins with a procedure prologue - setting up the call stack - and then is a
> series of fetch/stores to copy data from here to there
> 
> The memcpy() itself by its nature it not atomic on the front-side-bus of the
> APSS to shared DRAM with the firmware.
> 
> On a CPU and SoC architecture level this fix just doesn't work.
> 
> To be honest we are already doing the right thing in this routine.
> 
> 1. Reading the value from the packet header.
> 2. Validating the given size against the maximum size
> 3. Rejecting the memcpy() if the given size _at_the_time_we_read_ is too
>    large.
> 
> The alternative to guarantee would be something like
> 
> asm("bus master asserts bus lock to PAGE/PAGES involved");
> dwords = *rd_ptr;
> if (dwords > MAX_VALUE)
>     return -EFIRMWARE_BUG;
> memcpy(dst, src, dwords >> 2);
> 
> asm("bus master unlocks memory");
> 
> Lets say we make the change proposed in this patch, here is how it breaks:
> 
> if ((*rd_ptr >> 2) != dwords)
>     return -EINVAL;
> 
> // now trust dwords
> memcpy(pkt, rd_ptr, dwords << 2);
> 
> 
> objdump
> qlt-kernel/build/x1e80100-crd_qlt_integration/drivers/media/platform/qcom/venus/venus-core.o --disassemble=venus_read_queue.isra.0
> 
> 5c48:    540000c9     b.ls    5c60 <venus_read_queue.isra.0+0x110>  // b.plast
> 5c4c:    2a0303e2     mov    w2, w3
> 5c50:    aa0703e0     mov    x0, x7
> 5c54:    94000000     bl    0 <memcpy>
> 5c58:    52800000     mov    w0, #0x0
> 
> Your conditional jump is @ 0x5c48 your call to memcpy @ 0x5c54
> 
> Between 0x5c48 and 0x5c54 the firmware can update the value _again_
> Indeed the firmware can update the value up until the time we complete reading
> the bit of the pkt header in memcpy() so an additional few instructions for sure.
> 
> You could make some type of argument to re-verify the content of the pkt _after_
> the memcpy()
> 
> But the only verification that makes any sense _before_ the memcpy() is to
> verify the length at the point you _read_ - subsequent to the latching operation
> - were we fetch the length value from DRAM into our CPU cache, operating stack
> and/or local registers.
> 
> Once that data is fetched within the cache/stack/registers of the CPU/APSS that
> is the relevant value.
> 
> For the fix you have here to work you need this
> 
> 5c48:   MAGICOP         memorybuslock
> 5c48:    540000c9     b.ls    5c60 <venus_read_queue.isra.0+0x110>  // b.plast
> 5c4c:    2a0303e2     mov    w2, w3
> 5c50:    aa0703e0     mov    x0, x7
> 5c54:    94000000     bl    0 <memcpy>
> 5c58:    52800000     mov    w0, #0x0
> 5c5c:   MAGICUNOP       memorybusunlock
> 
> Because the firmware is free-running - with respect to the instruction pipeline
> of the above assembly.
> 
> If you really want to verify the data is still valid - it should be done _after_
> the memcpy();
> 
> But even then I'd say to you, why verify _after_ the memcpy() - and what happens
> on the instruction directly _after_ the verification - is the data considered
> more valid now ?
the patch _only_ reduces the window where data in shared queue can go wrong.
Doing it after memcpy() would be better here given the data would not be read
further from shared queue, which would avoid the case of data getting updated later.

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;

Regards,
Vikash
> 
> i.e. this:
> 
> memcpy(pkt, rd_ptr, dwords << 2);
> 
> if ((*rd_ptr >> 2) != dwords)
>     return -EINVAL;
> 
> doesn't have the above described architectural race condition but it doesn't
> make the data any more trustworthy - because it doesn't have atomicity
> 
> memcpy(pkt, rd_ptr, dwords << 2);
> 
> if ((*rd_ptr >> 2) != dwords)
>     return -EINVAL;
> 
> dev_info(dev, "The value of *rd_ptr %lu!=%lu can be different now\n",
>          *rd_ptr >> 2, dwords);
> 
> Sorry this patch just can't work. It's a very hard NAK from me.
> 
> ---
> bod

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

[Bryan O'Donoghue]

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.

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.

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 ?

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.

TBH I think the entire premise of this patch is bogus.

---
bod

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

[Vikash Garodia]

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

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

[Bryan O'Donoghue]

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

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

[Vikash Garodia]

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

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

[Bryan O'Donoghue]

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

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

[Vikash Garodia]

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

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

[Bryan O'Donoghue]

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

[PATCH v3 2/2] media: venus: Fix OOB read due to missing payload bound check

[Dikshita Agarwal]

From: Vedang Nagar <quic_vnagar@quicinc.com>

Currently, The event_seq_changed() handler processes a variable number
of properties sent by the firmware. The number of properties is indicated
by the firmware and used to iterate over the payload. However, the
payload size is not being validated against the actual message length.

This can lead to out-of-bounds memory access if the firmware provides a
property count that exceeds the data available in the payload. Such a
condition can result in kernel crashes or potential information leaks if
memory beyond the buffer is accessed.

Fix this by properly validating the remaining size of the payload before
each property access and updating bounds accordingly as properties are
parsed.

This ensures that property parsing is safely bounded within the received
message buffer and protects against malformed or malicious firmware
behavior.

Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
Co-developed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_msgs.c | 83 +++++++++++++++++++---------
 1 file changed, 58 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 0a041b4db9efc549621de07dd13b4a3a37a70d11..cf0d97cbc4631f907faf255a338ceca673eaab2b 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -33,8 +33,9 @@ static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
 	struct hfi_buffer_requirements *bufreq;
 	struct hfi_extradata_input_crop *crop;
 	struct hfi_dpb_counts *dpb_count;
+	u32 ptype, rem_bytes;
+	u32 size_read = 0;
 	u8 *data_ptr;
-	u32 ptype;
 
 	inst->error = HFI_ERR_NONE;
 
@@ -44,86 +45,118 @@ static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
 		break;
 	default:
 		inst->error = HFI_ERR_SESSION_INVALID_PARAMETER;
-		goto done;
+		inst->ops->event_notify(inst, EVT_SYS_EVENT_CHANGE, &event);
+		return;
 	}
 
 	event.event_type = pkt->event_data1;
 
 	num_properties_changed = pkt->event_data2;
-	if (!num_properties_changed) {
-		inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
-		goto done;
-	}
+	if (!num_properties_changed)
+		goto error;
 
 	data_ptr = (u8 *)&pkt->ext_event_data[0];
+	rem_bytes = pkt->shdr.hdr.size - sizeof(*pkt);
+
 	do {
+		if (rem_bytes < sizeof(u32))
+			goto error;
 		ptype = *((u32 *)data_ptr);
+
+		data_ptr += sizeof(u32);
+		rem_bytes -= sizeof(u32);
+
 		switch (ptype) {
 		case HFI_PROPERTY_PARAM_FRAME_SIZE:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_framesize))
+				goto error;
+
 			frame_sz = (struct hfi_framesize *)data_ptr;
 			event.width = frame_sz->width;
 			event.height = frame_sz->height;
-			data_ptr += sizeof(*frame_sz);
+			size_read = sizeof(struct hfi_framesize);
 			break;
 		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_profile_level))
+				goto error;
+
 			profile_level = (struct hfi_profile_level *)data_ptr;
 			event.profile = profile_level->profile;
 			event.level = profile_level->level;
-			data_ptr += sizeof(*profile_level);
+			size_read = sizeof(struct hfi_profile_level);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_PIXEL_BITDEPTH:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_bit_depth))
+				goto error;
+
 			pixel_depth = (struct hfi_bit_depth *)data_ptr;
 			event.bit_depth = pixel_depth->bit_depth;
-			data_ptr += sizeof(*pixel_depth);
+			size_read = sizeof(struct hfi_bit_depth);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_PIC_STRUCT:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_pic_struct))
+				goto error;
+
 			pic_struct = (struct hfi_pic_struct *)data_ptr;
 			event.pic_struct = pic_struct->progressive_only;
-			data_ptr += sizeof(*pic_struct);
+			size_read = sizeof(struct hfi_pic_struct);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_COLOUR_SPACE:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_colour_space))
+				goto error;
+
 			colour_info = (struct hfi_colour_space *)data_ptr;
 			event.colour_space = colour_info->colour_space;
-			data_ptr += sizeof(*colour_info);
+			size_read = sizeof(struct hfi_colour_space);
 			break;
 		case HFI_PROPERTY_CONFIG_VDEC_ENTROPY:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(u32))
+				goto error;
+
 			event.entropy_mode = *(u32 *)data_ptr;
-			data_ptr += sizeof(u32);
+			size_read = sizeof(u32);
 			break;
 		case HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_buffer_requirements))
+				goto error;
+
 			bufreq = (struct hfi_buffer_requirements *)data_ptr;
 			event.buf_count = hfi_bufreq_get_count_min(bufreq, ver);
-			data_ptr += sizeof(*bufreq);
+			size_read = sizeof(struct hfi_buffer_requirements);
 			break;
 		case HFI_INDEX_EXTRADATA_INPUT_CROP:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_extradata_input_crop))
+				goto error;
+
 			crop = (struct hfi_extradata_input_crop *)data_ptr;
 			event.input_crop.left = crop->left;
 			event.input_crop.top = crop->top;
 			event.input_crop.width = crop->width;
 			event.input_crop.height = crop->height;
-			data_ptr += sizeof(*crop);
+			size_read = sizeof(struct hfi_extradata_input_crop);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_DPB_COUNTS:
-			data_ptr += sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_dpb_counts))
+				goto error;
+
 			dpb_count = (struct hfi_dpb_counts *)data_ptr;
 			event.buf_count = dpb_count->fw_min_cnt;
-			data_ptr += sizeof(*dpb_count);
+			size_read = sizeof(struct hfi_dpb_counts);
 			break;
 		default:
+			size_read = 0;
 			break;
 		}
+		data_ptr += size_read;
+		rem_bytes -= size_read;
 		num_properties_changed--;
 	} while (num_properties_changed > 0);
 
-done:
+	inst->ops->event_notify(inst, EVT_SYS_EVENT_CHANGE, &event);
+	return;
+
+error:
+	inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
 	inst->ops->event_notify(inst, EVT_SYS_EVENT_CHANGE, &event);
 }
 

-- 
2.34.1

Re: [PATCH v3 2/2] media: venus: Fix OOB read due to missing payload bound check

[Bryan O'Donoghue]

On 14/05/2025 14:38, Dikshita Agarwal wrote:
> From: Vedang Nagar <quic_vnagar@quicinc.com>
> 
> Currently, The event_seq_changed() handler processes a variable number
> of properties sent by the firmware. The number of properties is indicated
> by the firmware and used to iterate over the payload. However, the
> payload size is not being validated against the actual message length.
> 
> This can lead to out-of-bounds memory access if the firmware provides a
> property count that exceeds the data available in the payload. Such a
> condition can result in kernel crashes or potential information leaks if
> memory beyond the buffer is accessed.
> 
> Fix this by properly validating the remaining size of the payload before
> each property access and updating bounds accordingly as properties are
> parsed.
> 
> This ensures that property parsing is safely bounded within the received
> message buffer and protects against malformed or malicious firmware
> behavior.
> 
> Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> Co-developed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v3 2/2] media: venus: Fix OOB read due to missing payload bound check

[Vikash Garodia]

On 5/14/2025 7:08 PM, Dikshita Agarwal wrote:
> From: Vedang Nagar <quic_vnagar@quicinc.com>
> 
> Currently, The event_seq_changed() handler processes a variable number
> of properties sent by the firmware. The number of properties is indicated
> by the firmware and used to iterate over the payload. However, the
> payload size is not being validated against the actual message length.
> 
> This can lead to out-of-bounds memory access if the firmware provides a
> property count that exceeds the data available in the payload. Such a
> condition can result in kernel crashes or potential information leaks if
> memory beyond the buffer is accessed.
> 
> Fix this by properly validating the remaining size of the payload before
> each property access and updating bounds accordingly as properties are
> parsed.
> 
> This ensures that property parsing is safely bounded within the received
> message buffer and protects against malformed or malicious firmware
> behavior.
> 
> Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> Co-developed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>