Message ID | 20230131030155.18932-1-akihiko.odaki@daynix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] softmmu: Use memmove in flatview_write_continue | expand |
On 31.01.23 04:01, Akihiko Odaki wrote: > We found a case where the source passed to flatview_write_continue() may > overlap with the destination when fuzzing igb, a new proposed network > device with sanitizers. > > igb uses pci_dma_map() to get Tx packet, and pci_dma_write() to write Rx > buffer. While pci_dma_write() is usually used to write data from > memory not mapped to the guest, if igb is configured to perform > loopback, the data will be sourced from the guest memory. The source and > destination can overlap and the usage of memcpy() will be invalid in > such a case. > > While we do not really have to deal with such an invalid request for > igb, detecting the overlap in igb code beforehand requires complex code, > and only covers this specific case. Instead, just replace memcpy() with > memmove() to tolerate overlaps. Using memmove() will slightly damage the > performance as it will need to check overlaps before using SIMD > instructions for copying, but the cost should be negligible, considering > the inherent complexity of flatview_write_continue(). > > The test cases generated by the fuzzer is available at: > https://patchew.org/QEMU/20230129053316.1071513-1-alxndr@bu.edu/ > > The fixed test case is: > fuzz/crash_47dfe62d9f911bf523ff48cd441b61c0013ed805 > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Acked-by: Alexander Bulekov <alxndr@bu.edu> > --- Acked-by: David Hildenbrand <david@redhat.com>
On 1/30/23 17:01, Akihiko Odaki wrote: > We found a case where the source passed to flatview_write_continue() may > overlap with the destination when fuzzing igb, a new proposed network > device with sanitizers. > > igb uses pci_dma_map() to get Tx packet, and pci_dma_write() to write Rx > buffer. While pci_dma_write() is usually used to write data from > memory not mapped to the guest, if igb is configured to perform > loopback, the data will be sourced from the guest memory. The source and > destination can overlap and the usage of memcpy() will be invalid in > such a case. > > While we do not really have to deal with such an invalid request for > igb, detecting the overlap in igb code beforehand requires complex code, > and only covers this specific case. Instead, just replace memcpy() with > memmove() to tolerate overlaps. Using memmove() will slightly damage the > performance as it will need to check overlaps before using SIMD > instructions for copying, but the cost should be negligible, considering > the inherent complexity of flatview_write_continue(). > > The test cases generated by the fuzzer is available at: > https://patchew.org/QEMU/20230129053316.1071513-1-alxndr@bu.edu/ > > The fixed test case is: > fuzz/crash_47dfe62d9f911bf523ff48cd441b61c0013ed805 > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Acked-by: Alexander Bulekov <alxndr@bu.edu> > --- > V1 -> V2: Correct spellings in the message Queued to tcg-next. r~ > > softmmu/physmem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index cb998cdf23..3cd27b1c9d 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -2828,7 +2828,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, > } else { > /* RAM case */ > ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false); > - memcpy(ram_ptr, buf, l); > + memmove(ram_ptr, buf, l); > invalidate_and_set_dirty(mr, addr1, l); > } >
diff --git a/softmmu/physmem.c b/softmmu/physmem.c index cb998cdf23..3cd27b1c9d 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2828,7 +2828,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, } else { /* RAM case */ ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false); - memcpy(ram_ptr, buf, l); + memmove(ram_ptr, buf, l); invalidate_and_set_dirty(mr, addr1, l); }