Re: [PATCH v2 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/2/2024 5:38 PM, Bryan O'Donoghue wrote:
> On 28/11/2024 05:05, Vikash Garodia wrote:
>> words_count denotes the number of words in total payload, while data
>> points to payload of various property within it. When words_count
>> reaches last word, data can access memory beyond the total payload. This
>> can lead to OOB access. Refactor the parsing logic such that the
>> remaining payload is checked before parsing it.
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@xxxxxxxxxxx>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_parser.c | 57 +++++++++++++++++++++-----
>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
>> b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index
>> 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..14349c2f84b205a8b79dee3acff1408bb63ac54a 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -282,8 +282,8 @@ static int hfi_platform_parser(struct venus_core *core,
>> struct venus_inst *inst)
>>   u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>              u32 size)
>>   {
>> +    u32 *words = buf, *payload, codecs = 0, domain = 0;
>>       unsigned int words_count = size >> 2;
>> -    u32 *word = buf, *data, codecs = 0, domain = 0;
>>       int ret;
>>         ret = hfi_platform_parser(core, inst);
>> @@ -301,36 +301,71 @@ u32 hfi_parser(struct venus_core *core, struct
>> venus_inst *inst, void *buf,
>>       }
>>         while (words_count) {
>> -        data = word + 1;
>> +        payload = words + 1;
>>   -        switch (*word) {
>> +        switch (*words) {
>>           case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>> -            parse_codecs(core, data);
>> +            if (words_count < sizeof(struct hfi_codec_supported))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_codecs(core, payload);
>>               init_codecs(core);
>> +            words_count -= sizeof(struct hfi_codec_supported);
>> +            words += sizeof(struct hfi_codec_supported);
>>               break;
>>           case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>> -            parse_max_sessions(core, data);
>> +            if (words_count < sizeof(struct hfi_max_sessions_supported))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_max_sessions(core, payload);
>> +            words_count -= sizeof(struct hfi_max_sessions_supported);
>> +            words += sizeof(struct hfi_max_sessions_supported);
>>               break;
>>           case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>> -            parse_codecs_mask(&codecs, &domain, data);
>> +            if (words_count < sizeof(struct hfi_codec_mask_supported))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_codecs_mask(&codecs, &domain, payload);
>> +            words_count -= sizeof(struct hfi_codec_mask_supported);
>> +            words += sizeof(struct hfi_codec_mask_supported);
>>               break;
>>           case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>> -            parse_raw_formats(core, codecs, domain, data);
>> +            if (words_count < sizeof(struct hfi_uncompressed_format_supported))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_raw_formats(core, codecs, domain, payload);
>> +            words_count -= sizeof(struct hfi_uncompressed_format_supported);
>> +            words += sizeof(struct hfi_uncompressed_format_supported);
>>               break;
>>           case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>> -            parse_caps(core, codecs, domain, data);
>> +            if (words_count < sizeof(struct hfi_capabilities))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_caps(core, codecs, domain, payload);
>> +            words_count -= sizeof(struct hfi_capabilities);
>> +            words += sizeof(struct hfi_capabilities);
>>               break;
>>           case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>> -            parse_profile_level(core, codecs, domain, data);
>> +            if (words_count < sizeof(struct hfi_profile_level_supported))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_profile_level(core, codecs, domain, payload);
>> +            words_count -= sizeof(struct hfi_profile_level_supported);
>> +            words += sizeof(struct hfi_profile_level_supported);
>>               break;
>>           case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>> -            parse_alloc_mode(core, codecs, domain, data);
>> +            if (words_count < sizeof(struct hfi_buffer_alloc_mode_supported))
>> +                return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +            parse_alloc_mode(core, codecs, domain, payload);
>> +            words_count -= sizeof(struct hfi_buffer_alloc_mode_supported);
>> +            words += sizeof(struct hfi_buffer_alloc_mode_supported);
>>               break;
>>           default:
>>               break;
>>           }
>>   -        word++;
>> +        words++;
>>           words_count--;
>>       }
>>  
> 
> I like the changes made here.
> 
> Let me suggest you have the parse_something() return the size of the buffer
> consumed or an error code.
> 
> If you calculate the maximum pointer instead of the words_count
> 
> frame_size = payload + max;
> 
> /* Your while can look like this */
> 
> while (words < frame_size)
> switch(*words){
> case HFI_PROPERTY_X:
>     /* if the function returns the bytes consumed */
>     ret = parse_x();
>     break;
> case HFI_PROPERTY_X:
>     ret = parse_x();
>     break;
> }
> 
> if (ret < 0)
>     return -ret;
> 
> /* you can increment the pointer once at the bottom of the loop */
> words += ret;
> }
> 
> 
> That way you can
> 
> 1. Get rid of words_count and not have to decrement it
> 2. Have one variable words which is checked against the maximum
>    size while(words < frame_size)
> 3. Have the function that consumes the data return
>    how much buffer it has consumed, instead of inlining in the
>    switch
> 4. Increment at the bottom of the switch once instead
>    of several times in the switch
> 
> IMO it would be clearer/neater that way. Please consider.
Appreciate your time to dig deeper into it. Expanding your suggestion, filling
in the details

frame_size = words + size;

/* Your while can look like this */

while (words < frame_size)
remaining_size = framesize - words;
switch(*words){
 case HFI_PROPERTY_X:
     if (remaining_size < sizeof(payload_X)
        return insuff_res;
     /* if the function returns the bytes consumed */
     ret = parse_x(core, words + 1);
     break;
 case HFI_PROPERTY_Y:
     if (remaining_size < sizeof(payload_X)
        return insuff_res;
     ret = parse_y(core, words + 1);
     break;
 default:
     ret = 1;
 }

 if (ret < 0)
     return -ret;

 /* you can increment the pointer once at the bottom of the loop */
 words += ret;
 }

If you see, words_count is doing the role of remaining_size. In existing
implementation as well, we can move those increments per case to once per loop,
just that to avoid incrementing for default case.

Regards,
Vikash




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux