| Message ID | 20210706161735.57967-1-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuzz: fix sparse memory access in the DMA callback | expand |
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>
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) { /*
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(-)