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? 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. thanks -- PMM