| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eed2f879-bdf3-b052-9259-4a4e28329fc6@quicinc.com>
Date: Tue, 13 May 2025 18:30:00 +0530
From: Vikash Garodia <quic_vgarodia@...cinc.com>
To: Vedang Nagar <quic_vnagar@...cinc.com>,
Stanimir Varbanov
<stanimir.k.varbanov@...il.com>,
Bryan O'Donoghue
<bryan.odonoghue@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] media: venus: fix OOB access issue while reading
sequence changed events
On 2/15/2025 10:49 PM, 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@...cinc.com>
> ---
> 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;
> + }
> +
> 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;
Instead of doing this for every switch, better to do it once before the switch.
> + if (rem_bytes < sizeof(struct hfi_framesize))
> + break;
> frame_sz = (struct hfi_framesize *)data_ptr;
> event.width = frame_sz->width;
> event.height = frame_sz->height;
> data_ptr += sizeof(*frame_sz);
> + rem_bytes -= sizeof(struct hfi_framesize);
for this one, lets store the size in a local variable, update for every switch,
and do the increment/decrement logic once at the end with that size. It avoids
doing the same logic for every switch.
Regards,
Vikash
> break;
> case HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_profile_level))
> + break;
> profile_level = (struct hfi_profile_level *)data_ptr;
> event.profile = profile_level->profile;
> event.level = profile_level->level;
> data_ptr += sizeof(*profile_level);
> + rem_bytes -= sizeof(struct hfi_profile_level);
> break;
> case HFI_PROPERTY_PARAM_VDEC_PIXEL_BITDEPTH:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_bit_depth))
> + break;
> pixel_depth = (struct hfi_bit_depth *)data_ptr;
> event.bit_depth = pixel_depth->bit_depth;
> data_ptr += sizeof(*pixel_depth);
> + rem_bytes -= sizeof(struct hfi_bit_depth);
> break;
> case HFI_PROPERTY_PARAM_VDEC_PIC_STRUCT:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_pic_struct))
> + break;
> pic_struct = (struct hfi_pic_struct *)data_ptr;
> event.pic_struct = pic_struct->progressive_only;
> data_ptr += sizeof(*pic_struct);
> + rem_bytes -= sizeof(struct hfi_pic_struct);
> break;
> case HFI_PROPERTY_PARAM_VDEC_COLOUR_SPACE:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_colour_space))
> + break;
> colour_info = (struct hfi_colour_space *)data_ptr;
> event.colour_space = colour_info->colour_space;
> data_ptr += sizeof(*colour_info);
> + rem_bytes -= sizeof(struct hfi_colour_space);
> break;
> case HFI_PROPERTY_CONFIG_VDEC_ENTROPY:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(u32))
> + break;
> event.entropy_mode = *(u32 *)data_ptr;
> data_ptr += sizeof(u32);
> + rem_bytes -= sizeof(u32);
> break;
> case HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_buffer_requirements))
> + break;
> bufreq = (struct hfi_buffer_requirements *)data_ptr;
> event.buf_count = hfi_bufreq_get_count_min(bufreq, ver);
> data_ptr += sizeof(*bufreq);
> + rem_bytes -= sizeof(struct hfi_buffer_requirements);
> break;
> case HFI_INDEX_EXTRADATA_INPUT_CROP:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_extradata_input_crop))
> + break;
> 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);
> + rem_bytes -= sizeof(struct hfi_extradata_input_crop);
> break;
> case HFI_PROPERTY_PARAM_VDEC_DPB_COUNTS:
> - data_ptr += sizeof(u32);
> + if (increment_data_ptr(data_ptr, &rem_bytes))
> + break;
> + if (rem_bytes < sizeof(struct hfi_dpb_counts))
> + break;
> dpb_count = (struct hfi_dpb_counts *)data_ptr;
> event.buf_count = dpb_count->fw_min_cnt;
> data_ptr += sizeof(*dpb_count);
> + rem_bytes -= sizeof(struct hfi_dpb_counts);
> break;
> default:
> break;
>
Powered by blists - more mailing lists