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

Open Source and information security mailing list archives

Message-ID: <c678cf02-700d-48c0-4160-8ee248a26da5@arm.com>
Date:   Wed, 22 Nov 2017 13:15:08 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Eric Yang <yu.yang_3@....com>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Kees Cook <keescook@...omium.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>
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@...cle.com]
>> Sent: Tuesday, November 21, 2017 12:27 AM
>> To: Eric Yang <yu.yang_3@....com>; iommu@...ts.linux-foundation.org
>> Cc: linux-kernel@...r.kernel.org; Greg Kroah-Hartman
>> <gregkh@...uxfoundation.org>; Andrew Morton <akpm@...ux-foundation.org>;
>> Andrey Ryabinin <aryabinin@...tuozzo.com>; David Miller
>> <davem@...emloft.net>; Ingo Molnar <mingo@...nel.org>; Geert
>> Uytterhoeven <geert+renesas@...der.be>; Al Viro <viro@...iv.linux.org.uk>;
>> Kees Cook <keescook@...omium.org>; Daniel Borkmann
>> <daniel@...earbox.net>
>> 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@...an 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@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives