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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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