| Message ID | 20250215-venus-security-fixes-v2-2-cfc7e4b87168@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | venus driver fixes to avoid possible OOB read access | expand |
On 3/4/2025 7:05 PM, Bryan O'Donoghue wrote: > 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@quicinc.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; >> + } > > This doesn't make sense. > > u32 rem_bytes; > > if (rem_bytes - 4 < 0) > Would be better to replace it with if (rem_bytes < sizeof(u32)) this make sure that rem_bytes contain some valid packet. > >> + >> 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 ? > Make sense. we should set the inst->error instead of silently breaking from loop. Will handle this in next revision. Thanks, Dikshita > --- > bod >
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@quicinc.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; >
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; + 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); 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;
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@quicinc.com> --- drivers/media/platform/qcom/venus/hfi_msgs.c | 72 ++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 10 deletions(-)