* [PATCH] softmmu: Use memmove in flatview_write_continue
@ 2023-01-30 13:51 Akihiko Odaki
2023-01-30 16:37 ` Peter Xu
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Akihiko Odaki @ 2023-01-30 13:51 UTC (permalink / raw)
Cc: qemu-devel, Philippe Mathieu-Daudé, David Hildenbrand,
Peter Xu, Paolo Bonzini, Alexander Bulekov, Akihiko Odaki
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 torelate 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 negiligble, 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>
---
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);
}
--
2.39.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] softmmu: Use memmove in flatview_write_continue
2023-01-30 13:51 [PATCH] softmmu: Use memmove in flatview_write_continue Akihiko Odaki
@ 2023-01-30 16:37 ` Peter Xu
2023-01-30 20:03 ` Alexander Bulekov
2023-01-30 23:03 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2023-01-30 16:37 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, Philippe Mathieu-Daudé, David Hildenbrand,
Paolo Bonzini, Alexander Bulekov
On Mon, Jan 30, 2023 at 10:51:52PM +0900, 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 torelate 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 negiligble, 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: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] softmmu: Use memmove in flatview_write_continue
2023-01-30 13:51 [PATCH] softmmu: Use memmove in flatview_write_continue Akihiko Odaki
2023-01-30 16:37 ` Peter Xu
@ 2023-01-30 20:03 ` Alexander Bulekov
2023-01-30 20:28 ` Peter Xu
2023-01-30 23:03 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 6+ messages in thread
From: Alexander Bulekov @ 2023-01-30 20:03 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, Philippe Mathieu-Daudé, David Hildenbrand,
Peter Xu, Paolo Bonzini
On 230130 2251, 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 torelate 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 negiligble, 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>
Since this is called fairly often, I went down a rabit hole looking at
memmove vs memcpy perf, but It looks like this should not be much of a
problem.
Acked-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Thank you
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> 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);
> }
>
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] softmmu: Use memmove in flatview_write_continue
2023-01-30 20:03 ` Alexander Bulekov
@ 2023-01-30 20:28 ` Peter Xu
2023-01-30 23:49 ` Alexander Bulekov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Xu @ 2023-01-30 20:28 UTC (permalink / raw)
To: Alexander Bulekov
Cc: Akihiko Odaki, qemu-devel, Philippe Mathieu-Daudé,
David Hildenbrand, Paolo Bonzini
On Mon, Jan 30, 2023 at 03:03:00PM -0500, Alexander Bulekov wrote:
> On 230130 2251, 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 torelate 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 negiligble, 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>
>
> Since this is called fairly often, I went down a rabit hole looking at
> memmove vs memcpy perf, but It looks like this should not be much of a
> problem.
Similar here. I quickly had a look at the dump of memmove() and it seems to
me it's directly invoking the SIMD instructions. I'm not sure whether it
means in many cases the SIMD insts are compatible with overlapped buffers
already, so it can be mostly the same as memcpy() in modern hardwares.
>
> Acked-by: Akihiko Odaki <akihiko.odaki@daynix.com>
I received this reply from Alexander Bulekov, but I think this is the
author's contact?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] softmmu: Use memmove in flatview_write_continue
2023-01-30 20:28 ` Peter Xu
@ 2023-01-30 23:49 ` Alexander Bulekov
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Bulekov @ 2023-01-30 23:49 UTC (permalink / raw)
To: Peter Xu
Cc: Akihiko Odaki, qemu-devel, Philippe Mathieu-Daudé,
David Hildenbrand, Paolo Bonzini
On 230130 1528, Peter Xu wrote:
> On Mon, Jan 30, 2023 at 03:03:00PM -0500, Alexander Bulekov wrote:
> > On 230130 2251, 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 torelate 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 negiligble, 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>
> >
> > Since this is called fairly often, I went down a rabit hole looking at
> > memmove vs memcpy perf, but It looks like this should not be much of a
> > problem.
>
> Similar here. I quickly had a look at the dump of memmove() and it seems to
> me it's directly invoking the SIMD instructions. I'm not sure whether it
> means in many cases the SIMD insts are compatible with overlapped buffers
> already, so it can be mostly the same as memcpy() in modern hardwares.
>
> >
> > Acked-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> I received this reply from Alexander Bulekov, but I think this is the
> author's contact?
Sorry about that!
Acked-by: Alexander Bulekov <alxndr@bu.edu>
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] softmmu: Use memmove in flatview_write_continue
2023-01-30 13:51 [PATCH] softmmu: Use memmove in flatview_write_continue Akihiko Odaki
2023-01-30 16:37 ` Peter Xu
2023-01-30 20:03 ` Alexander Bulekov
@ 2023-01-30 23:03 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-30 23:03 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, David Hildenbrand, Peter Xu, Paolo Bonzini,
Alexander Bulekov
On 30/1/23 14:51, 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 torelate overlaps. Using memmove() will slightly damage the
"tolerate"?
> performance as it will need to check overlaps before using SIMD
> instructions for copying, but the cost should be negiligble, considering
"negligible"
> 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>
> ---
> 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);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-30 23:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-30 13:51 [PATCH] softmmu: Use memmove in flatview_write_continue Akihiko Odaki
2023-01-30 16:37 ` Peter Xu
2023-01-30 20:03 ` Alexander Bulekov
2023-01-30 20:28 ` Peter Xu
2023-01-30 23:49 ` Alexander Bulekov
2023-01-30 23:03 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).