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..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 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);
 		}
+		memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
 	} else {
 		/* bad packet received, dropping */
 		new_rd_idx = qhdr->write_idx;

-- 
2.34.1

On 04/01/2025 05:41, 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.

A brief scan of this code doesn't really show the base case you assert here.

Could you be a bit more explicit.

  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..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 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);
>   		}
> +		memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));

I'm not entirely following your reasoning here.

Here's how the code looks after your change:

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));

Which of the above memcpy() operations is subject to a pointer that 
firmware can influence exactly ?

Is this a real problem you've seen if so please add a backtrace to this 
commit log.

>   	} else {
>   		/* bad packet received, dropping */
>   		new_rd_idx = qhdr->write_idx;
> 

If this is a fix it requires a Fixes: tag.

Please add.

Still finding the reasoning you are outlining here not obvious.

---
bod

Hi Bryan,

On 1/6/2025 5:28 AM, Bryan O'Donoghue wrote:
> On 04/01/2025 05:41, 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.
> 
> A brief scan of this code doesn't really show the base case you assert here.
> 
> Could you be a bit more explicit.
> 
>  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..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 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);
>>           }
>> +        memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
> 
> I'm not entirely following your reasoning here.
> 
> Here's how the code looks after your change:
> 
> 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));
> 
> Which of the above memcpy() operations is subject to a pointer that firmware can influence exactly ?
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.

To avoid the issue, planning to repopulate the pkt with the first read like below:
 *(u32 *)pkt = dwords << 2;
Pls ignore the original solution which seems to be not correct.
Pls let me know if it looks good, will fix it in next version.

Regards,
Vedang Nagar
> 
> Is this a real problem you've seen if so please add a backtrace to this commit log.
> 
>>       } else {
>>           /* bad packet received, dropping */
>>           new_rd_idx = qhdr->write_idx;
>>
> 
> If this is a fix it requires a Fixes: tag.
> 
> Please add.
> 
> Still finding the reasoning you are outlining here not obvious.
> 
> ---
> bod

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 ?

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.

What exactly is the valid lifetime of this data from HFI RX interrupt ?

---
bod

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