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

[Bryan O'Donoghue]

On 15/02/2025 17:19, 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. 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@xxxxxxxxxxx>
> ---
>   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..c124db8ac79d18f32289a690ee82145dc93daee6 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);
>   		}
> +		*(u32 *)pkt = dwords << 2;
>   	} else {
>   		/* bad packet received, dropping */
>   		new_rd_idx = qhdr->write_idx;

This is confusing - where is the read

Your previous code

https://lore.kernel.org/lkml/20250104-venus-security-fixes-v1-1-9d0dd4594cb4@xxxxxxxxxxx/

memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));

V1 then would have been:

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

V2 proposed:

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

+ *(u32 *)pkt = dwords << 2;

My comment wasn't about use of memcpy() it was about why we are doing this.

For example if new_rd_idx < qsize is true then we literally do

a) memcpy(pkt, rd_ptr, dwords << 2);
b) *(u32 *)pkt = dwords << 2;

and the question is why ? That is an unambiguous cast of pkt to the 
value of dwords << 2;

What is the scope of how the data can change from a to b ?

And why is the data considered potentially invalid @ the memcpy() but 
valid subsequent the cast ?

Assuming rd_ptr contains the length of dwords << 2 to begin with in the 
first 4 bytes - why is it necessary to make _really_ _really_ sure by 
restuffing the data ?

For example if *(u32 *)rd_ptr != dwords << 2 - why shouldn't we just 
throw the whole frame away as containing junk data ?

---
bod