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

[Robin Murphy]

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.

> > > [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://lists.linuxfoundation.org/mailman/listinfo/iommu