RE: No check of the size passed to unmap_single in swiotlb

[Eric Yang]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: Wednesday, November 22, 2017 9:15 PM
> To: Eric Yang <yu.yang_3@xxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>; Kees Cook
> <keescook@xxxxxxxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; David Miller <davem@xxxxxxxxxxxxx>; Al Viro
> <viro@xxxxxxxxxxxxxxxxxx>; Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx>;
> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>; Ingo Molnar
> <mingo@xxxxxxxxxx>
> Subject: Re: No check of the size passed to unmap_single in swiotlb
> 
> On 22/11/17 03:23, Eric Yang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> >> Sent: Tuesday, November 21, 2017 12:27 AM
> >> To: Eric Yang <yu.yang_3@xxxxxxx>; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; Greg Kroah-Hartman
> >> <gregkh@xxxxxxxxxxxxxxxxxxx>; Andrew Morton
> >> <akpm@xxxxxxxxxxxxxxxxxxxx>; Andrey Ryabinin
> >> <aryabinin@xxxxxxxxxxxxx>; David Miller <davem@xxxxxxxxxxxxx>; Ingo
> >> Molnar <mingo@xxxxxxxxxx>; Geert Uytterhoeven
> >> <geert+renesas@xxxxxxxxx>; Al Viro <viro@xxxxxxxxxxxxxxxxxx>; Kees
> >> Cook <keescook@xxxxxxxxxxxx>; Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> >> Subject: Re: No check of the size passed to unmap_single in swiotlb
> >>
> >> On Mon, Nov 20, 2017 at 08:17:14AM +0000, Eric Yang wrote:
> >>> Hi all,
> >>
> >> Hi!
> >
> > Hi  Konrad,
> >>>
> >>> During debug a  device only support 32bits DMA(Qualcomm Atheros AP)
> >>> in our
> >> LS1043A 64bits ARM  SOC, we found that the invoke of dma_unmap_single
> >> --> swiotlb_tbl_unmap_single  will unmap the passed "size" anyway
> >> even when the "size" is incorrect.
> >>>
> >>> If the size is larger than it should, the extra entries in
> >>> io_tlb_orig_addr array
> >> will be refilled by INVALID_PHYS_ADDR, and it will cause the bounce
> >> buffer copy not happen when the one who really used the mis-freed
> >> entries doing  DMA data transfers, and this will cause further unknow
> behaviors.
> >>>
> >>> Here we just fix it temporarily by adding a judge of the "size" in
> >>> the
> >> swiotlb_tbl_unmap_single, if it is larger than it deserves, just
> >> unmap the right size only. Like the code:
> >>
> >> Did the DMA debug API (CONFIG_DMA_API_DEBUG) help in figuring this
> >> issue as well?
> >>
> >
> > I have just enabled this config and move off the kernel fix and test,
> > there seems the debug API has no output for this size incorrect issue.
> > For I only enable the config and no other operations and my kernel is 4.9, do I
> missed something?
> >
> > I confirm that the debug API is working, for there are other drivers
> > triggered its warning output
> > like:
> >
> > caam_jr 1710000.jr: DMA-API: device driver tries to free DMA memory it
> > has not allocated [device address=0x6800458400000000]
> 
> By default, dma-debug will only log the first error it sees - you can adjust this
> with the "num_errors" or "all_errors" sysfs controls, and use the driver filter to
> hide errors from other drivers if necessary.
> 
> Robin.
> 
Hi Robin,

I've caught the size mismatch warning with the debug API, thanks a lot for the help.

Regards,
Eric

> >>>
> >>> [yangyu@titan dash-lts]$ git diff ./lib/swiotlb.c diff --git
> >>> a/lib/swiotlb.c b/lib/swiotlb.c index ad1d2962d129..58c97ede9d78
> >>> 100644
> >>> --- a/lib/swiotlb.c
> >>> +++ b/lib/swiotlb.c
> >>> @@ -591,7 +591,10 @@ void swiotlb_tbl_unmap_single(struct device
> >>> *hwdev,
> >> phys_addr_t tlb_addr,
> >>>                   */
> >>>                  for (i = index + nslots - 1; i >= index; i--) {
> >>>                          io_tlb_list[i] = ++count;
> >>> -                       io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> >>> +                      if(io_tlb_orig_addr[i]  != orig_addr)
> >>> +                          printk("======size wrong, ally down ally down!===\n");
> >>> +                      else
> >>> +                         io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> >>>                  }
> >>>                  /*
> >>>                   * Step 2: merge the returned slots with the
> >>> preceding slots,
> >>>
> >>> Although pass a right size of DMA buffer is the responsibility of
> >>> the drivers, but
> >> Is it useful to add some size check code to prevent  real damage happen?
> >>>
> >>> Regards,
> >>> Eric
> > _______________________________________________
> > iommu mailing list
> > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> >
> ts.linuxfoundation.org%2Fmailman%2Flistinfo%2Fiommu&data=02%7C01%7Cyu.
> >
> yang_3%40nxp.com%7Cf3851e3c8ae24f341b3608d531ab1165%7C686ea1d3bc
> 2b4c6f
> >
> a92cd99c5c301635%7C0%7C0%7C636469533155971955&sdata=7PnqUlgRlJq2H
> sXmTf
> > CmEUuWaBfUhzSd33UoK4li1Ro%3D&reserved=0
> >