On Mon May 19 12:42:22 2025 +0530, Vedang Nagar wrote:
> Currently, The event_seq_changed() handler processes a variable number
> of properties sent by the firmware. The number of properties is indicated
> by the firmware and used to iterate over the payload. However, the
> payload size is not being validated against the actual message length.
>
> This can lead to out-of-bounds memory access if the firmware provides a
> property count that exceeds the data available in the payload. Such a
> condition can result in kernel crashes or potential information leaks if
> memory beyond the buffer is accessed.
>
> Fix this by properly validating the remaining size of the payload before
> each property access and updating bounds accordingly as properties are
> parsed.
>
> This ensures that property parsing is safely bounded within the received
> message buffer and protects against malformed or malicious firmware
> behavior.
>
> Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface
> (HFI)")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Vedang Nagar <quic_vna...@quicinc.com>
> Reviewed-by: Vikash Garodia <quic_vgaro...@quicinc.com>
> Reviewed-by: Bryan O'Donoghue <bryan.odonog...@linaro.org>
> Co-developed-by: Dikshita Agarwal <quic_diksh...@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_diksh...@quicinc.com>
> Signed-off-by: Bryan O'Donoghue <b...@kernel.org>
> Signed-off-by: Hans Verkuil <hverk...@xs4all.nl>
Patch committed.
Thanks,
Hans Verkuil
drivers/media/platform/qcom/venus/hfi_msgs.c | 83 +++++++++++++++++++---------
1 file changed, 58 insertions(+), 25 deletions(-)
---
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c
b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 0a041b4db9ef..cf0d97cbc463 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -33,8 +33,9 @@ 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;
+ u32 size_read = 0;
u8 *data_ptr;
- u32 ptype;
inst->error = HFI_ERR_NONE;
@@ -44,86 +45,118 @@ static void event_seq_changed(struct venus_core *core,
struct venus_inst *inst,
break;
default:
inst->error = HFI_ERR_SESSION_INVALID_PARAMETER;
- goto done;
+ inst->ops->event_notify(inst, EVT_SYS_EVENT_CHANGE, &event);
+ return;
}
event.event_type = pkt->event_data1;
num_properties_changed = pkt->event_data2;
- if (!num_properties_changed) {
- inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
- goto done;
- }
+ if (!num_properties_changed)
+ goto error;
data_ptr = (u8 *)&pkt->ext_event_data[0];
+ rem_bytes = pkt->shdr.hdr.size - sizeof(*pkt);
+
do {
+ if (rem_bytes < sizeof(u32))
+ goto error;
ptype = *((u32 *)data_ptr);
+
+ data_ptr += sizeof(u32);
+ rem_bytes -= sizeof(u32);
+
switch (ptype) {
case HFI_PROPERTY_PARAM_FRAME_SIZE:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_framesize))
+ goto error;
+
frame_sz = (struct hfi_framesize *)data_ptr;
event.width = frame_sz->width;
event.height = frame_sz->height;
- data_ptr += sizeof(*frame_sz);
+ size_read = sizeof(struct hfi_framesize);
break;
case HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_profile_level))
+ goto error;
+
profile_level = (struct hfi_profile_level *)data_ptr;
event.profile = profile_level->profile;
event.level = profile_level->level;
- data_ptr += sizeof(*profile_level);
+ size_read = sizeof(struct hfi_profile_level);
break;
case HFI_PROPERTY_PARAM_VDEC_PIXEL_BITDEPTH:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_bit_depth))
+ goto error;
+
pixel_depth = (struct hfi_bit_depth *)data_ptr;
event.bit_depth = pixel_depth->bit_depth;
- data_ptr += sizeof(*pixel_depth);
+ size_read = sizeof(struct hfi_bit_depth);
break;
case HFI_PROPERTY_PARAM_VDEC_PIC_STRUCT:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_pic_struct))
+ goto error;
+
pic_struct = (struct hfi_pic_struct *)data_ptr;
event.pic_struct = pic_struct->progressive_only;
- data_ptr += sizeof(*pic_struct);
+ size_read = sizeof(struct hfi_pic_struct);
break;
case HFI_PROPERTY_PARAM_VDEC_COLOUR_SPACE:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_colour_space))
+ goto error;
+
colour_info = (struct hfi_colour_space *)data_ptr;
event.colour_space = colour_info->colour_space;
- data_ptr += sizeof(*colour_info);
+ size_read = sizeof(struct hfi_colour_space);
break;
case HFI_PROPERTY_CONFIG_VDEC_ENTROPY:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(u32))
+ goto error;
+
event.entropy_mode = *(u32 *)data_ptr;
- data_ptr += sizeof(u32);
+ size_read = sizeof(u32);
break;
case HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_buffer_requirements))
+ goto error;
+
bufreq = (struct hfi_buffer_requirements *)data_ptr;
event.buf_count = hfi_bufreq_get_count_min(bufreq, ver);
- data_ptr += sizeof(*bufreq);
+ size_read = sizeof(struct hfi_buffer_requirements);
break;
case HFI_INDEX_EXTRADATA_INPUT_CROP:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_extradata_input_crop))
+ goto error;
+
crop = (struct hfi_extradata_input_crop *)data_ptr;
event.input_crop.left = crop->left;
event.input_crop.top = crop->top;
event.input_crop.width = crop->width;
event.input_crop.height = crop->height;
- data_ptr += sizeof(*crop);
+ size_read = sizeof(struct hfi_extradata_input_crop);
break;
case HFI_PROPERTY_PARAM_VDEC_DPB_COUNTS:
- data_ptr += sizeof(u32);
+ if (rem_bytes < sizeof(struct hfi_dpb_counts))
+ goto error;
+
dpb_count = (struct hfi_dpb_counts *)data_ptr;
event.buf_count = dpb_count->fw_min_cnt;
- data_ptr += sizeof(*dpb_count);
+ size_read = sizeof(struct hfi_dpb_counts);
break;
default:
+ size_read = 0;
break;
}
+ data_ptr += size_read;
+ rem_bytes -= size_read;
num_properties_changed--;
} while (num_properties_changed > 0);
-done:
+ inst->ops->event_notify(inst, EVT_SYS_EVENT_CHANGE, &event);
+ return;
+
+error:
+ inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
inst->ops->event_notify(inst, EVT_SYS_EVENT_CHANGE, &event);
}
