| Message ID | 20241105-venus_oob-v1-2-8d4feedfe2bb@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Venus driver fixes to avoid possible OOB accesses | expand |
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
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(-)