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

[Alexandra Diupina]

17/04/24 13:05, Konrad, Frederic пишет:
> Hi,
>
> > -----Original Message-----
> > From: qemu-devel-bounces+fkonrad=amd.com@nongnu.org 
> > <qemu-devel-bounces+fkonrad=amd.com@nongnu.org> On Behalf Of
> > Peter Maydell
> > Sent: Friday, April 12, 2024 12:07 PM
> > To: Alexandra Diupina <adiupina@astralinux.ru>
> > Cc: Alistair Francis <alistair@alistair23.me>; Edgar E. Iglesias 
> > <edgar.iglesias@gmail.com>; qemu-arm@nongnu.org; qemu-
> > devel@nongnu.org; sdl.qemu@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 <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?
> 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