Re: [PATCH v2] softmmu: Use memmove in flatview_write_continue

[Richard Henderson <richard.henderson@linaro.org>]

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);
>           }
>