Re: [PATCH RFC] prevent overflow in xlnx_dpdma_desc_get_source_address()

[Peter Maydell]

On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina <adiupina@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 <adiupina@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