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

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

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
>