| Message ID | 20250104-venus-security-fixes-v1-2-9d0dd4594cb4@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | venus driver fixes to avoid possible OOB read access | expand |
On 04/01/2025 05:41, 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> Please see Vikash's series on this. https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/ it seems to have exactly the same patch title ? Is this patch supposed to be a follow-up to that patch ? https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/ Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here. On the switch statement I'd have two comments. #1 is everything really a " -= sizeof(u32)" ? #2 if so then this ought to be factored out into a function => functional decomposition --- bod
Hi Bryan, On 1/17/2025 4:02 PM, Bryan O'Donoghue wrote: > On 17/01/2025 08:39, Vedang Nagar wrote: >> Hi Bryan, >> >> On 1/6/2025 5:36 AM, Bryan O'Donoghue wrote: >>> On 04/01/2025 05:41, 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> >>> Please see Vikash's series on this. >>> >>> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/ >>> >>> it seems to have exactly the same patch title ? >>> >>> Is this patch supposed to be a follow-up to that patch ? >>> >>> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/ >>> >>> Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here. >> No, this is a different series having OOB fixes similar to ones posted by Vikash. > > OK, please use a different patch title. > > I understand the motive to repeat the patch title but, its confusing. If you added some text to make the OOB more specific then it would be possible to differentiate between. > > "fix OOB access issue while reading sequence changed events 'in some location' || 'on some path'" Got it, will update the patch title in the next version. > > >>> >>> On the switch statement I'd have two comments. >>> >>> #1 is everything really a " -= sizeof(u32)" ? >> Yes, it's everytime " -= sizeof(u32) " since the first the first word read is ptype of size u32 >>> #2 if so then this ought to be factored out into a function >>> => functional decomposition >> Sure, will fix this with decomposition into functions. > > Is firmware sending a change event or updating a packet already in memory ? Firmware is sending a change event in a new packet. > > What is the nature of the change event and how do you guarantee the second read is valid when the first read can be considered invalid ? > > i.e. > > - Read - derive read value X. > - Do some stuff. > - Read - again to make sure length value is still X. > - Do all sorts of other processing. > > At which point is the sequence considered complete and the data considered "locked" and valid ? With rouge firmware, the data can get invalid by the firmware during the second read but our intention is to avoid reading the out of bound data. Whereas reading the invalid data will eventually lead to session_error at some point in future. > > What happens if you get a subsequent change event once this procedure has completed ? Either instance will go into error state or the same process will be repeated. Please let us know if this is not a correct approach, or any other suggestions. Regards, Vedang Nagar > > --- > bod
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c index 0a041b4db9efc549621de07dd13b4a3a37a70d11..3fff21ea744b0171e204dd0851fc46cb468e1979 100644 --- a/drivers/media/platform/qcom/venus/hfi_msgs.c +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c @@ -33,8 +33,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 +56,126 @@ 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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: + if (rem_bytes < sizeof(u32)) + break; data_ptr += sizeof(u32); + rem_bytes -= sizeof(u32); + 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 | 62 +++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-)