[PATCH-for-5.2 0/3] Bug-fixes for the generic-fuzzer

[PATCH-for-5.2 0/3] Bug-fixes for the generic-fuzzer

[Alexander Bulekov]

These patches fix some silly issues I found after the generic-fuzzer
started running on OSS-Fuzz.

Alexander Bulekov (3):
  fuzz: fix writing DMA patterns
  fuzz: check the MR in the DMA callback
  fuzz: fuzz offsets within pio/mmio regions

 tests/qtest/fuzz/generic_fuzz.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)

-- 
2.28.0

[PATCH-for-5.2 1/3] fuzz: fix writing DMA patterns

[Alexander Bulekov]

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(-)

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

Re: [PATCH-for-5.2 1/3] fuzz: fix writing DMA patterns

[Alexander Bulekov]

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
> 

[PATCH-for-5.2 2/3] fuzz: check the MR in the DMA callback

[Alexander Bulekov]

We should be checking that the device is trying to read from RAM, before
filling the region with data. Otherwise, we will try to populate
nonsensical addresses in RAM for callbacks on PIO/MMIO reads. We did
this originally, however the final version I sent had the line commented
out..

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index 3e2d50feaa..31f24ad06f 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -192,7 +192,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write)
      */
     if (dma_patterns->len == 0
         || len == 0
-        /* || mr != MACHINE(qdev_get_machine())->ram */
+        || mr != MACHINE(qdev_get_machine())->ram
         || is_write
         || addr > current_machine->ram_size) {
         return;
-- 
2.28.0

[PATCH-for-5.2 3/3] fuzz: fuzz offsets within pio/mmio regions

[Alexander Bulekov]

The code did not add offsets to FlatRange bases, so we did not fuzz
offsets within device MemoryRegions.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index 31f24ad06f..5770f86be6 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -298,6 +298,11 @@ static bool get_io_address(address_range *result, AddressSpace *as,
     } while (cb_info.index != index && !cb_info.found);
 
     *result = cb_info.result;
+    if (result->size) {
+        offset = offset % result->size;
+        result->addr += offset;
+        result->size -= offset;
+    }
     return cb_info.found;
 }
 
-- 
2.28.0

Re: [PATCH-for-5.2 0/3] Bug-fixes for the generic-fuzzer

[Darren Kenny]

Sigh, I should have caught some of these in the last review, sorry.

For the series,

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

On Thursday, 2020-10-29 at 13:28:57 -04, Alexander Bulekov wrote:
> These patches fix some silly issues I found after the generic-fuzzer
> started running on OSS-Fuzz.
>
> Alexander Bulekov (3):
>   fuzz: fix writing DMA patterns
>   fuzz: check the MR in the DMA callback
>   fuzz: fuzz offsets within pio/mmio regions
>
>  tests/qtest/fuzz/generic_fuzz.c | 44 +++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
>
> -- 
> 2.28.0