| Message ID | 20241105-venus_oob-v1-2-8d4feedfe2bb@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Venus driver fixes to avoid possible OOB accesses | expand |
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@vger.kernel.org >> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser") >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> --- >> 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
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
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) {
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@vger.kernel.org Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser") Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> --- drivers/media/platform/qcom/venus/hfi_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)