From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751529AbdKVNPO (ORCPT ); Wed, 22 Nov 2017 08:15:14 -0500 Received: from foss.arm.com ([217.140.101.70]:51928 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751244AbdKVNPM (ORCPT ); Wed, 22 Nov 2017 08:15:12 -0500 Subject: Re: No check of the size passed to unmap_single in swiotlb To: Eric Yang , Konrad Rzeszutek Wilk , "iommu@lists.linux-foundation.org" Cc: Daniel Borkmann , Kees Cook , Geert Uytterhoeven , Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , David Miller , Al Viro , Andrey Ryabinin , Andrew Morton , Ingo Molnar References: <20171120162652.GR24312@char.us.oracle.com> From: Robin Murphy Message-ID: Date: Wed, 22 Nov 2017 13:15:08 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22/11/17 03:23, Eric Yang wrote: > > >> -----Original Message----- >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] >> Sent: Tuesday, November 21, 2017 12:27 AM >> To: Eric Yang ; iommu@lists.linux-foundation.org >> Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman >> ; Andrew Morton ; >> Andrey Ryabinin ; David Miller >> ; Ingo Molnar ; Geert >> Uytterhoeven ; Al Viro ; >> Kees Cook ; Daniel Borkmann >> >> 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@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu >