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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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

---
bod