The code mistakenly relied on address_space_translate to store the
length remaining until the next memory-region. We care about this
because when there is RAM or sparse-memory neighboring on an MMIO
region, we should only write up to the border, to prevent inadvertently
invoking MMIO handlers within the DMA callback.

However address_space_translate_internal only stores the length until
the end of the MemoryRegion if memory_region_is_ram(mr). Otherwise
the *len is left unmodified. This caused some false-positive issues,
where the fuzzer found a way to perform a nested MMIO write through a
DMA callback on an [address, length] that started within sparse memory
and spanned some device MMIO regions.

To fix this, write to sparse memory in small chunks of
memory_access_size (similar to the underlying address_space_write code),
which will prevent accidentally hitting MMIO handlers through large
writes.

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

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index 59be7219de..3e8ce29227 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -240,10 +240,17 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr)
                                       addr, &addr1, &l, true,
                                       MEMTXATTRS_UNSPECIFIED);
 
-        if (!(memory_region_is_ram(mr1) ||
-              memory_region_is_romd(mr1)) && mr1 != sparse_mem_mr) {
+        /*
+         *  If mr1 isn't RAM, address_space_translate doesn't update l. Use
+         *  memory_access_size to identify the number of bytes that it is safe
+         *  to write without accidentally writing to another MemoryRegion.
+         */
+        if (!memory_region_is_ram(mr1)) {
             l = memory_access_size(mr1, l, addr1);
-        } else {
+        }
+        if (memory_region_is_ram(mr1) ||
+            memory_region_is_romd(mr1) ||
+            mr1 == sparse_mem_mr) {
             /* ROM/RAM case */
             if (qtest_log_enabled) {
                 /*
-- 
2.28.0

On Tuesday, 2021-07-06 at 12:17:35 -04, Alexander Bulekov wrote:
> The code mistakenly relied on address_space_translate to store the
> length remaining until the next memory-region. We care about this
> because when there is RAM or sparse-memory neighboring on an MMIO
> region, we should only write up to the border, to prevent inadvertently
> invoking MMIO handlers within the DMA callback.
>
> However address_space_translate_internal only stores the length until
> the end of the MemoryRegion if memory_region_is_ram(mr). Otherwise
> the *len is left unmodified. This caused some false-positive issues,
> where the fuzzer found a way to perform a nested MMIO write through a
> DMA callback on an [address, length] that started within sparse memory
> and spanned some device MMIO regions.
>
> To fix this, write to sparse memory in small chunks of
> memory_access_size (similar to the underlying address_space_write code),
> which will prevent accidentally hitting MMIO handlers through large
> writes.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

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

> ---
>  tests/qtest/fuzz/generic_fuzz.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index 59be7219de..3e8ce29227 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -240,10 +240,17 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr)
>                                        addr, &addr1, &l, true,
>                                        MEMTXATTRS_UNSPECIFIED);
>  
> -        if (!(memory_region_is_ram(mr1) ||
> -              memory_region_is_romd(mr1)) && mr1 != sparse_mem_mr) {
> +        /*
> +         *  If mr1 isn't RAM, address_space_translate doesn't update l. Use
> +         *  memory_access_size to identify the number of bytes that it is safe
> +         *  to write without accidentally writing to another MemoryRegion.
> +         */
> +        if (!memory_region_is_ram(mr1)) {
>              l = memory_access_size(mr1, l, addr1);
> -        } else {
> +        }
> +        if (memory_region_is_ram(mr1) ||
> +            memory_region_is_romd(mr1) ||
> +            mr1 == sparse_mem_mr) {
>              /* ROM/RAM case */
>              if (qtest_log_enabled) {
>                  /*
> -- 
> 2.28.0

On 7/6/21 6:17 PM, Alexander Bulekov wrote:
> The code mistakenly relied on address_space_translate to store the
> length remaining until the next memory-region. We care about this
> because when there is RAM or sparse-memory neighboring on an MMIO
> region, we should only write up to the border, to prevent inadvertently
> invoking MMIO handlers within the DMA callback.
> 
> However address_space_translate_internal only stores the length until
> the end of the MemoryRegion if memory_region_is_ram(mr). Otherwise
> the *len is left unmodified. This caused some false-positive issues,
> where the fuzzer found a way to perform a nested MMIO write through a
> DMA callback on an [address, length] that started within sparse memory
> and spanned some device MMIO regions.
> 
> To fix this, write to sparse memory in small chunks of
> memory_access_size (similar to the underlying address_space_write code),
> which will prevent accidentally hitting MMIO handlers through large
> writes.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/generic_fuzz.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>