Re: [PATCH v2 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Bryan O'Donoghue]

On 15/02/2025 17:19, Vedang Nagar wrote:
> num_properties_changed is being read from the message queue but is
> not validated. Value can be corrupted from the firmware leading to
> OOB read access issues. Add fix to read the size of the packets as
> well and crosscheck before reading from the packet.
>
> Signed-off-by: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
> ---
>   drivers/media/platform/qcom/venus/hfi_msgs.c | 72 ++++++++++++++++++++++++----
>   1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index 0a041b4db9efc549621de07dd13b4a3a37a70d11..2ad60a3fbfe0286de09a44664fc3b30259aa0368 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -19,6 +19,16 @@
>   #define VER_STR_SZ		128
>   #define SMEM_IMG_OFFSET_VENUS	(14 * 128)
>   
> +static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
> +{
> +	if (*rem_bytes < sizeof(u32))
> +		return -EINVAL;
> +	data_ptr += sizeof(u32);
> +	*rem_bytes -= sizeof(u32);
> +
> +	return 0;
> +}
> +
>   static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>   			      struct hfi_msg_event_notify_pkt *pkt)
>   {
> @@ -33,8 +43,8 @@ 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;
>   	u8 *data_ptr;
> -	u32 ptype;
>   
>   	inst->error = HFI_ERR_NONE;
>   
> @@ -56,66 +66,108 @@ static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>   	}
>   
>   	data_ptr = (u8 *)&pkt->ext_event_data[0];
> +	rem_bytes = pkt->shdr.hdr.size - sizeof(*pkt);
> +	if (rem_bytes - 4 < 0) {
> +		inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
> +		goto done;
> +	}

This doesn't make sense.

u32 rem_bytes;

if (rem_bytes - 4 < 0)


> +
>   	do {
>   		ptype = *((u32 *)data_ptr);
>   		switch (ptype) {
>   		case HFI_PROPERTY_PARAM_FRAME_SIZE:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			if (rem_bytes < sizeof(struct hfi_framesize))
> +				break;

In every case you are return -EINVAL but not setting

inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;

surely that is a natural and logical outcome of running out of buffer 
and a fact you'd want to communicate outside of the driver, rather than 
silent failing in this switch ?

---
bod