Re: [PATCH v2] softmmu: Use memmove in flatview_write_continue
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-alx...@bu.edu/ The fixed test case is: fuzz/crash_47dfe62d9f911bf523ff48cd441b61c0013ed805 Signed-off-by: Akihiko Odaki Acked-by: Alexander Bulekov --- 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, , false); -memcpy(ram_ptr, buf, l); +memmove(ram_ptr, buf, l); invalidate_and_set_dirty(mr, addr1, l); }
Re: [PATCH v2] softmmu: Use memmove in flatview_write_continue
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-alx...@bu.edu/ The fixed test case is: fuzz/crash_47dfe62d9f911bf523ff48cd441b61c0013ed805 Signed-off-by: Akihiko Odaki Acked-by: Alexander Bulekov --- Acked-by: David Hildenbrand -- Thanks, David / dhildenb
[PATCH v2] softmmu: Use memmove in flatview_write_continue
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-alx...@bu.edu/ The fixed test case is: fuzz/crash_47dfe62d9f911bf523ff48cd441b61c0013ed805 Signed-off-by: Akihiko Odaki Acked-by: Alexander Bulekov --- V1 -> V2: Correct spellings in the message 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, , false); -memcpy(ram_ptr, buf, l); +memmove(ram_ptr, buf, l); invalidate_and_set_dirty(mr, addr1, l); } -- 2.39.1