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) {
--
2.34.1
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; 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/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
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
On 12/11/2024 08:05, Vikash Garodia wrote: > 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. Right but the loop _can't_ be correct. What's the point in fixing an OOB in a loop that doesn't work ? This is the loop: #define BUF_SIZE 0x20 // BUF_SIZE doesn't really matter char somebuf[BUF_SIZE]; u32 *word = somebuf[0]; u32 words = ARRAY_SIZE(somebuf); while (words > 1) { data = word + 1; // this word++; // and this words--; } On the first loop word = somebuf[0]; data = somebuf[3]; On the second loop word = somebuf[3]; // the same value as *data in the previous loop and that's just broken because on the second loop *word == *data in the first loop ! That's what my program showed you word 4 == 0x03020100 data=0x07060504 // word == data from previous loop word 3 == 0x07060504 data=0x0b0a0908 // word == data from previous loop word 2 == 0x0b0a0908 data=0x0f0e0d0c The step size, the number of bytes this loop increments is fundamentally wrong because a) Its a fixed size [1] b) *word in loop(n+1) == *data in loop(n) Which cannot ever parse more than one data item - in effect never loop - in one go. > 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 [1] But we've established that word increments by one word. We wouldn't fix this loop by just making it into while (words > 1) { data = word + 1; word = data + 1; words -= 2; } Because the consumers of the data have different step sizes, different number of bytes they consume for the structs they cast. => case HFI_PROPERTY_PARAM_CODEC_SUPPORTED: parse_codecs(core, data); // consumes sizeof(struct hfi_codec_supported) struct hfi_codec_supported { u32 dec_codecs; u32 enc_codecs; }; case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED: parse_max_sessions(core, data); // consumes sizeof(struct hfi_max_sessions_supported) struct hfi_max_sessions_supported { u32 max_sessions; }; case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED: parse_codecs_mask(&codecs, &domain, data); // consumes sizeof(struct hfi_codec_mask_supported) struct hfi_codec_mask_supported { u32 codecs; u32 video_domains; }; case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED: parse_raw_formats(core, codecs, domain, data); // consumes sizeof(struct hfi_uncompressed_format_supported) struct hfi_uncompressed_format_supported { u32 buffer_type; u32 format_entries; struct hfi_uncompressed_plane_info plane_info; }; case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED: parse_caps(core, codecs, domain, data); struct hfi_capabilities { u32 num_capabilities; struct hfi_capability data[]; }; where hfi_platform.h:#define MAX_CAP_ENTRIES 32 I'll stop there. This routine needs a rewrite. --- bod
On 11/12/2024 4:47 PM, Bryan O'Donoghue wrote: > On 12/11/2024 08:05, Vikash Garodia wrote: >> 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. > > Right but the loop _can't_ be correct. What's the point in fixing an OOB in a > loop that doesn't work ? > > This is the loop: > > #define BUF_SIZE 0x20 // BUF_SIZE doesn't really matter > > char somebuf[BUF_SIZE]; > u32 *word = somebuf[0]; > u32 words = ARRAY_SIZE(somebuf); > > while (words > 1) { > data = word + 1; // this > word++; // and this > words--; > } > > On the first loop > word = somebuf[0]; > data = somebuf[3]; > > On the second loop > word = somebuf[3]; // the same value as *data in the previous loop > > and that's just broken because on the second loop *word == *data in the first > loop ! > > That's what my program showed you > > word 4 == 0x03020100 data=0x07060504 > > // word == data from previous loop > word 3 == 0x07060504 data=0x0b0a0908 > > // word == data from previous loop > word 2 == 0x0b0a0908 data=0x0f0e0d0c > > The step size, the number of bytes this loop increments is fundamentally wrong > because > > a) Its a fixed size [1] > b) *word in loop(n+1) == *data in loop(n) > > Which cannot ever parse more than one data item - in effect never loop - in one go. In the second iteration, the loop would not match with any case and would try to match the case by incrementing word. Let say the first word is "HFI_PROPERTY_PARAM_CODEC_SUPPORTED" followed by 2 words (second and third word) of payload step size. At this point, now when the loop runs again with second word and third word, it would not match any case. Again at 4th word, it would match a case and process the payload. One thing that we can do here is to increment the word count with the step size of the data consumed ? This way 2nd and 3rd iteration can be skipped as we know that there would not be any case in those words. Regards, Vikash > >> 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 > > [1] > > But we've established that word increments by one word. > We wouldn't fix this loop by just making it into > > while (words > 1) { > data = word + 1; > word = data + 1; > words -= 2; > } > > Because the consumers of the data have different step sizes, different number of > bytes they consume for the structs they cast. > > => > > case HFI_PROPERTY_PARAM_CODEC_SUPPORTED: > parse_codecs(core, data); > // consumes sizeof(struct hfi_codec_supported) > struct hfi_codec_supported { > u32 dec_codecs; > u32 enc_codecs; > }; > > > case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED: > parse_max_sessions(core, data); > // consumes sizeof(struct hfi_max_sessions_supported) > struct hfi_max_sessions_supported { > u32 max_sessions; > }; > > case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED: > parse_codecs_mask(&codecs, &domain, data); > // consumes sizeof(struct hfi_codec_mask_supported) > struct hfi_codec_mask_supported { > u32 codecs; > u32 video_domains; > }; > > case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED: > parse_raw_formats(core, codecs, domain, data); > // consumes sizeof(struct hfi_uncompressed_format_supported) > struct hfi_uncompressed_format_supported { > u32 buffer_type; > u32 format_entries; > struct hfi_uncompressed_plane_info plane_info; > }; > > case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED: > parse_caps(core, codecs, domain, data); > > struct hfi_capabilities { > u32 num_capabilities; > struct hfi_capability data[]; > }; > > where > hfi_platform.h:#define MAX_CAP_ENTRIES 32 > > I'll stop there. > > This routine needs a rewrite. > > --- > bod
On 12/11/2024 12:58, Vikash Garodia wrote: > One thing that we can do here is to increment the word count with the step size > of the data consumed ? I think that's the right thing to do. --- bod
© 2016 - 2024 Red Hat, Inc.