On 11/5/2024 4:45 PM, Bryan O'Donoghue wrote:
> On 05/11/2024 08:54, 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.
>> Avoid this case by not allowing the loop for the last word count.
>>
>> 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 | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
>> b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index
>> 27d0172294d5154f4839e8cef172f9a619dfa305..20d9ea3626e9c4468d5f7dbd678743135f027c86 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -303,7 +303,7 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst
>> *inst, void *buf,
>> memset(core->caps, 0, sizeof(core->caps));
>> }
>> - while (words_count) {
>> + while (words_count > 1) {
>> data = word + 1;
>> switch (*word) {
>>
>
> How is it the right thing to do to _not_ process the last u32 ?
>
> How does this overrun ? while (words_count) should be fine because it decrements
> at the bottom of the loop...
>
> assuming your buffer is word aligned obvs
>
> =>
>
> #include <stdio.h>
> #include <stdint.h>
>
> char somebuf[64];
>
> 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;
Make this as below and it should lead to OOB
uint32_t *my_word = (uint32_t*)buf + 1
Regards,
Vikash
>
> printf("Size %d word_count %d\n", size, word_count);
>
> while(word_count) {
> printf("Myword %d == 0x%08x\n", word_count, *my_word);
> 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
> 10 = 10
> 11 = 11
> 12 = 12
> 13 = 13
> 14 = 14
> 15 = 15
> 16 = 16
> 17 = 17
> 18 = 18
> 19 = 19
> 1a = 1a
> 1b = 1b
> 1c = 1c
> 1d = 1d
> 1e = 1e
> 1f = 1f
> 20 = 20
> 21 = 21
> 22 = 22
> 23 = 23
> 24 = 24
> 25 = 25
> 26 = 26
> 27 = 27
> 28 = 28
> 29 = 29
> 2a = 2a
> 2b = 2b
> 2c = 2c
> 2d = 2d
> 2e = 2e
> 2f = 2f
> 30 = 30
> 31 = 31
> 32 = 32
> 33 = 33
> 34 = 34
> 35 = 35
> 36 = 36
> 37 = 37
> 38 = 38
> 39 = 39
> 3a = 3a
> 3b = 3b
> 3c = 3c
> 3d = 3d
> 3e = 3e
> 3f = 3f
> Size 64 word_count 16
> Myword 16 == 0x03020100
> Myword 15 == 0x07060504
> Myword 14 == 0x0b0a0908
> Myword 13 == 0x0f0e0d0c
> Myword 12 == 0x13121110
> Myword 11 == 0x17161514
> Myword 10 == 0x1b1a1918
> Myword 9 == 0x1f1e1d1c
> Myword 8 == 0x23222120
> Myword 7 == 0x27262524
> Myword 6 == 0x2b2a2928
> Myword 5 == 0x2f2e2d2c
> Myword 4 == 0x33323130
> Myword 3 == 0x37363534
> Myword 2 == 0x3b3a3938
> Myword 1 == 0x3f3e3d3c
>
> ---
> bod
