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

[Vikash Garodia <quic_vgarodia@quicinc.com>]

On 11/12/2024 5:13 AM, Bryan O'Donoghue wrote:
> On 11/11/2024 14:36, Vikash Garodia wrote:
>>> int hfi_parser(void *buf, int size)
>>> {
>>>          int word_count = size >> 2;
>>>          uint32_t*my_word = (uint32_t*)buf;
>> Make this as below and it should lead to OOB
>> uint32_t*my_word = (uint32_t*)buf + 1
>>
>> Regards,
>> Vikash
> 
> How does this code make sense ?
> 
> 
>         while (words_count) {
>                 data = word + 1;
> 
>                 switch (*word) {
>                 case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>                         parse_codecs(core, data);
>                         init_codecs(core);
>                         break;
>                 case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>                         parse_max_sessions(core, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>                         parse_codecs_mask(&codecs, &domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>                         parse_raw_formats(core, codecs, domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>                         parse_caps(core, codecs, domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>                         parse_profile_level(core, codecs, domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>                         parse_alloc_mode(core, codecs, domain, data);
>                         break;
>                 default:
>                         break;
>                 }
> 
>                 word++;
>                 words_count--;
>         }
> 
> 
> word[] = { 0, 1, 2, 3 };
> 
> words_count = 4;
> 
> while(words_count);
> 
>     data = word + 1;
> 
>     switch(*word) {
>     case WHATEVER:
>         do_something(param, data);
>     }
> 
>     word++;
>     words_count--;
> }
> 
> // iteration 0
> data = 1;
> *word = 0;
> 
> // iteration 1
> data = 2;
> *word = 1;
> 
> ????
> 
> How can the step size of word be correct ?
> 
> Do we ever actually process more than one pair here ?
> 
> #include <stdio.h>
> #include <stdint.h>
> 
> char somebuf[16];
> 
> void init(char *buf, int len)
> {
>         int i;
>         char c = 0;
> 
>         for (i = 0; i < len; i++)
>                 buf[i] = c++;
> }
> 
> int hfi_parser(void *buf, int size)
> {
>         int word_count = size >> 2;
>         uint32_t *my_word = (uint32_t*)buf, *data;
> 
>         printf("Size %d word_count %d\n", size, word_count);
> 
>         while (word_count > 1) {
>                 data = my_word + 1;
>                 printf("Myword %d == 0x%08x data=0x%08x\n", word_count,
> *my_word, *data);
>                 my_word++;
>                 word_count--;
>         }
> }
> 
> int main(int argc, char *argv[])
> {
>         int i;
> 
>         init(somebuf, sizeof(somebuf));
>         for (i = 0; i < sizeof(somebuf); i++)
>                 printf("%x = %x\n", i, somebuf[i]);
> 
>         hfi_parser(somebuf, sizeof(somebuf));
> 
>         return 0;
> }
> 
> 0 = 0
> 1 = 1
> 2 = 2
> 3 = 3
> 4 = 4
> 5 = 5
> 6 = 6
> 7 = 7
> 8 = 8
> 9 = 9
> a = a
> b = b
> c = c
> d = d
> e = e
> f = f
> Size 16 word_count 4
> Myword 4 == 0x03020100 data=0x07060504
> Myword 3 == 0x07060504 data=0x0b0a0908
> Myword 2 == 0x0b0a0908 data=0x0f0e0d0c
You did not printed the last iteration without the proposed fix. In the last
iteration (Myword 1), it would access the data beyond allocated size of somebuf.
So we can see how the fix protects from OOB situation.

For the functionality part, packet from firmware would come as <prop type>
followed by <payload for that prop> i.e
*word = HFI_PROPERTY_PARAM_CODEC_SUPPORTED
*data = payload --> hence here data is pointed to next u32 to point and parse
payload for HFI_PROPERTY_PARAM_CODEC_SUPPORTED.
likewise for other properties in the same packet

Regards
Vikash