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

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4989 bytes --]

+ To: Fred

On Tue, 16 Apr 2024 at 19:56, Alexandra Diupina <adiupina@astralinux.ru>
wrote:

> Peter, thank you! I agree with you that
> as mentioned in the documentation
> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field,
> we should take 32 bits of the address from one field
> (for example, case 1, SRC_ADDR2_EXT - in code it is desc->source_address2)
> and 16 bits (high or low) of the address from another field
> (ADDR_EXT_23 - in code it is desc->address_extension_23, we need [15:0]
> bits)
> and combine them to make a 48 bit address.
>
> Therefore, I suggest making the following changes to the code
> so that it matches the documentation:
>
> static uint64_t xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
>                                                       uint8_t frag)
> {
>      uint64_t addr = 0;
>      assert(frag < 5);
>
>      switch (frag) {
>      case 0:
>          addr = (uint64_t)desc->source_address
>              + (extract64(desc->address_extension, 16, 16) << 32);
>          break;
>      case 1:
>          addr = (uint64_t)desc->source_address2
>              + (extract64(desc->address_extension_23, 0, 16) << 32);
>          break;
>      case 2:
>          addr = (uint64_t)desc->source_address3
>              + (extract64(desc->address_extension_23, 16, 16) << 32);
>          break;
>      case 3:
>          addr = (uint64_t)desc->source_address4
>              + (extract64(desc->address_extension_45, 0, 16) << 32);
>          break;
>      case 4:
>          addr = (uint64_t)desc->source_address5
>              + (extract64(desc->address_extension_45, 16, 16) << 32);
>          break;
>      default:
>          addr = 0;
>          break;
>      }
>
>      return addr;
> }
>
>
> This change adds a type cast and also uses extract64() instead of
> extract32()
> to avoid integer overflow on addition (there was a typo in the previous
> letter).
> Also in extract64() extracts a bit field with a length of 16 bits
> instead of 12,
> the shift is changed to 32 so that the extracted field fits into bits
> [47:32] of the final address.
>
> if this calculation is correct, I'm ready to create a second version of
> the patch.
>
>
>
>
> 12/04/24 13:06, 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
>
>
>

[-- Attachment #2: Type: text/html, Size: 6697 bytes --]