17/04/24 13:05, Konrad, Frederic пишет:
Hi,
-----Original Message-----
From: qemu-devel-bounces+fkonrad=amd....@nongnu.org
<qemu-devel-bounces+fkonrad=amd....@nongnu.org> On Behalf Of
Peter Maydell
Sent: Friday, April 12, 2024 12:07 PM
To: Alexandra Diupina <adiup...@astralinux.ru>
Cc: Alistair Francis <alist...@alistair23.me>; Edgar E. Iglesias
<edgar.igles...@gmail.com>; qemu-...@nongnu.org; qemu-
de...@nongnu.org; sdl.q...@linuxtesting.org
Subject: Re: [PATCH RFC] prevent overflow in
xlnx_dpdma_desc_get_source_address()
On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiup...@astralinux.ru> wrote:
Overflow can occur in a situation where desc->source_address
has a maximum value (pow(2, 32) - 1), so add a cast to a
larger type before the assignment.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina <adiup...@astralinux.ru>
---
hw/dma/xlnx_dpdma.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..224259225c 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
- addr = desc->source_address
- + (extract32(desc->address_extension, 16, 12) << 20);
+ addr = (uint64_t)(desc->source_address
+ + (extract32(desc->address_extension, 16, 12) << 20));
Unless I'm confused, this cast doesn't help, because we
will have already done a 32-bit addition of desc->source_address
and the value from the address_extension part, so it doesn't
change the result.
If we want to do the addition at 64 bits then using extract64()
would be the simplest way to arrange for that.
However, I can't figure out what this code is trying to do and
make that line up with the data sheet; maybe this isn't the
right datasheet for this device?
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
The datasheet suggests that we should take 32 bits of the address
from one field (here desc->source_address) and 16 bits of the
address from another (here desc->address_extension's high bits)
and combine them to make a 48 bit address. But this code is only
looking at 12 bits of the high 16 in address_extension, and it
doesn't shift them right enough to put them into bits [47:32]
of the final address.
Xilinx folks: what hardware is this modelling, and is it
really the right behaviour?
Looks like this is the right documentation. Most probably the descriptor field
changed
since I did that model, or I got really confused.
Also, this device looks like it has a host-endianness bug: the
DPDMADescriptor struct is read directly from guest memory in
dma_memory_read(), but the device never does anything to swap
the fields from guest memory order to host memory order. So
this is likely broken on big-endian hosts.
Yes indeed.
Best Regards,
Fred
thanks
-- PMM
hw/dma/xlnx_dpdma.c defines a dma_ops structure
with the endianness field set to DEVICE_NATIVE_ENDIAN.
Doesn't this guarantee that there will be no endian errors?
Alexandra