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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c9783a99-724a-cdf0-7e76-7cbf2c77d63f@quicinc.com>
Date: Tue, 12 Nov 2024 13:35:39 +0530
From: Vikash Garodia <quic_vgarodia@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Stanimir Varbanov
	<stanimir.k.varbanov@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond
 payload word count


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