| Message ID | 20201029172901.534442-2-alxndr@bu.edu |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bug-fixes for the generic-fuzzer | expand |
On 201029 1328, Alexander Bulekov wrote: > This code had all sorts of issues. We used a loop similar to > address_space_write_rom, but I did not remove a "break" that only made > sense in the context of the switch statement in the original code. Then, > after the loop, we did a separate qtest_memwrite over the entire DMA > access range, defeating the purpose of the loop. Additionally, we > increment the buf pointer, and then try to g_free() it. Fix these > problems. > > Reported-by: OSS-Fuzz (Issue 26725) Also: Reported-by: OSS-Fuzz (Issue 26691) > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > --- > tests/qtest/fuzz/generic_fuzz.c | 37 +++++++++++++++------------------ > 1 file changed, 17 insertions(+), 20 deletions(-) > > diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c > index a8f5864883..3e2d50feaa 100644 > --- a/tests/qtest/fuzz/generic_fuzz.c > +++ b/tests/qtest/fuzz/generic_fuzz.c > @@ -229,10 +229,10 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) > address_range ar = {addr, len}; > g_array_append_val(dma_regions, ar); > pattern p = g_array_index(dma_patterns, pattern, dma_pattern_index); > - void *buf = pattern_alloc(p, ar.size); > + void *buf_base = pattern_alloc(p, ar.size); > + void *buf = buf_base; > hwaddr l, addr1; > MemoryRegion *mr1; > - uint8_t *ram_ptr; > while (len > 0) { > l = len; > mr1 = address_space_translate(first_cpu->as, > @@ -244,30 +244,27 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) > l = memory_access_size(mr1, l, addr1); > } else { > /* ROM/RAM case */ > - ram_ptr = qemu_map_ram_ptr(mr1->ram_block, addr1); > - memcpy(ram_ptr, buf, l); > - break; > + if (qtest_log_enabled) { > + /* > + * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log > + * that will be written by qtest.c with a DMA tag, so we can reorder > + * the resulting QTest trace so the DMA fills precede the last PIO/MMIO > + * command. > + */ > + fprintf(stderr, "[DMA] "); > + if (double_fetch) { > + fprintf(stderr, "[DOUBLE-FETCH] "); > + } > + fflush(stderr); > + } > + qtest_memwrite(qts_global, addr, buf, l); > } > len -= l; > buf += l; > addr += l; > > } > - if (qtest_log_enabled) { > - /* > - * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log > - * that will be written by qtest.c with a DMA tag, so we can reorder > - * the resulting QTest trace so the DMA fills precede the last PIO/MMIO > - * command. > - */ > - fprintf(stderr, "[DMA] "); > - if (double_fetch) { > - fprintf(stderr, "[DOUBLE-FETCH] "); > - } > - fflush(stderr); > - } > - qtest_memwrite(qts_global, ar.addr, buf, ar.size); > - g_free(buf); > + g_free(buf_base); > > /* Increment the index of the pattern for the next DMA access */ > dma_pattern_index = (dma_pattern_index + 1) % dma_patterns->len; > -- > 2.28.0 >
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index a8f5864883..3e2d50feaa 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -229,10 +229,10 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) address_range ar = {addr, len}; g_array_append_val(dma_regions, ar); pattern p = g_array_index(dma_patterns, pattern, dma_pattern_index); - void *buf = pattern_alloc(p, ar.size); + void *buf_base = pattern_alloc(p, ar.size); + void *buf = buf_base; hwaddr l, addr1; MemoryRegion *mr1; - uint8_t *ram_ptr; while (len > 0) { l = len; mr1 = address_space_translate(first_cpu->as, @@ -244,30 +244,27 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) l = memory_access_size(mr1, l, addr1); } else { /* ROM/RAM case */ - ram_ptr = qemu_map_ram_ptr(mr1->ram_block, addr1); - memcpy(ram_ptr, buf, l); - break; + if (qtest_log_enabled) { + /* + * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log + * that will be written by qtest.c with a DMA tag, so we can reorder + * the resulting QTest trace so the DMA fills precede the last PIO/MMIO + * command. + */ + fprintf(stderr, "[DMA] "); + if (double_fetch) { + fprintf(stderr, "[DOUBLE-FETCH] "); + } + fflush(stderr); + } + qtest_memwrite(qts_global, addr, buf, l); } len -= l; buf += l; addr += l; } - if (qtest_log_enabled) { - /* - * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log - * that will be written by qtest.c with a DMA tag, so we can reorder - * the resulting QTest trace so the DMA fills precede the last PIO/MMIO - * command. - */ - fprintf(stderr, "[DMA] "); - if (double_fetch) { - fprintf(stderr, "[DOUBLE-FETCH] "); - } - fflush(stderr); - } - qtest_memwrite(qts_global, ar.addr, buf, ar.size); - g_free(buf); + g_free(buf_base); /* Increment the index of the pattern for the next DMA access */ dma_pattern_index = (dma_pattern_index + 1) % dma_patterns->len;
This code had all sorts of issues. We used a loop similar to address_space_write_rom, but I did not remove a "break" that only made sense in the context of the switch statement in the original code. Then, after the loop, we did a separate qtest_memwrite over the entire DMA access range, defeating the purpose of the loop. Additionally, we increment the buf pointer, and then try to g_free() it. Fix these problems. Reported-by: OSS-Fuzz (Issue 26725) Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- tests/qtest/fuzz/generic_fuzz.c | 37 +++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-)