Re: [PATCH v4 1/2] media: venus: Add a check for packet size after reading from shared memory

[Bryan O'Donoghue]

On 19/05/2025 08:12, Dikshita Agarwal wrote:
> From: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
>
> Add a check to ensure that the packet size does not exceed the number of
> available words after reading the packet header from shared memory. This
> ensures that the size provided by the firmware is safe to process and
> prevent potential out-of-bounds memory access.
>
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
> Co-developed-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index b5f2ea8799507f9b83f1529e70061ea89a9cc5c8..c982f4527bb0b9f9ef9715c6c1dc26729f0fc079 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -239,6 +239,7 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
>   static int venus_read_queue(struct venus_hfi_device *hdev,
>   			    struct iface_queue *queue, void *pkt, u32 *tx_req)
>   {
> +	struct hfi_pkt_hdr *pkt_hdr = NULL;
>   	struct hfi_queue_header *qhdr;
>   	u32 dwords, new_rd_idx;
>   	u32 rd_idx, wr_idx, type, qsize;
> @@ -304,6 +305,9 @@ 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);
>   		}
> +		pkt_hdr = (struct hfi_pkt_hdr *)(pkt);
> +		if ((pkt_hdr->size >> 2) != dwords)
> +			return -EINVAL;
>   	} else {
>   		/* bad packet received, dropping */
>   		new_rd_idx = qhdr->write_idx;

Yes, validating the pkt header against the expected/used dwords is a 
valid check.

One thing I'm finding difficult to do with this code is get straight in 
my head why we are continuously shifting >> 2 and then in the other 
direction << 2

Surely a candidate fragment for some rationalisation down to

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

I count six shifts in twenty LOC or at least four shifts more than we 
should be doing if we just worked this over a bit.

Anyway.

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