linux-kernel - Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <10ccf249-8f2e-a6b3-2536-b78b417300a9@osg.samsung.com>
Date:   Thu, 13 Apr 2017 17:42:56 -0600
From:   Shuah Khan <shuahkh@....samsung.com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     gregkh@...uxfoundation.org, jroedel@...e.de, will.deacon@....com,
        Robin.Murphy@....com, vgupta@...opsys.com,
        m.szyprowski@...sung.com, bart.vanassche@...disk.com,
        benjamin.gaignard@...aro.org, krzk@...nel.org,
        niklas.soderlund+renesas@...natech.se, sricharan@...eaurora.org,
        vinod.koul@...el.com, akpm@...ux-foundation.org,
        dledford@...hat.com, alexander.h.duyck@...el.com,
        mauricfo@...ux.vnet.ibm.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory
 without struct page

On 04/13/2017 04:10 PM, Russell King - ARM Linux wrote:
> On Thu, Apr 13, 2017 at 03:47:56PM -0600, Shuah Khan wrote:
>> When coherent DMA memory without struct page is shared, importer
>> fails to find the page and runs into kernel page fault when it
>> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
>> in the sg_table.
>>
>> Add a new dma_check_dev_coherent() interface to check if memory is
>> from the device coherent area. There is no way to tell where the
>> memory returned by dma_alloc_attrs() came from.
>>
>> arm_dma_get_sgtable() checks for invalid pages, however this check
>> could pass even for memory obtained the coherent allocator. Add an
>> additional check to call dma_check_dev_coherent() to confirm that it
>> is indeed the coherent DMA memory and fail the sgtable creation with
>> -EINVAL.
> 
> Sorry, this doesn't make much sense to me.
> 
> pfn_valid(pfn) must *never* return true if 'pfn' does not have a struct
> page associated with it.  If it returns true (so we allow
> arm_dma_get_sgtable() to succeed) then we know we have a valid struct
> page in the supplied scatterlist.
> 
>> Signed-off-by: Shuah Khan <shuahkh@....samsung.com>
>> ---
>>  arch/arm/mm/dma-mapping.c   | 11 ++++++++---
>>  drivers/base/dma-coherent.c | 25 +++++++++++++++++++++++++
>>  include/linux/dma-mapping.h |  2 ++
>>  3 files changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index 475811f..27c7d9a 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -954,9 +954,14 @@ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>>  	struct page *page;
>>  	int ret;
>>  
>> -	/* If the PFN is not valid, we do not have a struct page */
>> -	if (!pfn_valid(pfn))
>> -		return -ENXIO;
>> +	/*
>> +	 * If the PFN is not valid, we do not have a struct page
>> +	 * As this check can pass even for memory obtained through
>> +	 * the coherent allocator, do an additional check to determine
>> +	 * if this is coherent DMA memory.
>> +	 */
>> +	if (!pfn_valid(pfn) && dma_check_dev_coherent(dev, handle, cpu_addr))
>> +		return -EINVAL;
> 
> Right, so what this says is:
> 
> 	if we do not haev a valid PFN
> 	_and_ if the memory is from the coherent section
> 	_then_ fail
> 
> Why the extra check?  Under what circunstances do we end up with memory
> where the PFN is valid, but we do not have a valid struct page.  It
> seems to me that such a scenario is a bug in pfn_valid() and not
> something that should be worked around like this.
> 

DMA_ATTR_NO_KERNEL_MAPPING case is the one I am concerned about.
pfn_valid() would fail on this if I am understanding it correctly.
A few drm drivers set this attr and use sg_table for passing buffers.

My reasoning behind adding this check is to not have this fail on
NO_KERNEL_MAPPING cases. So I thought adding a restrictive check for
just the per-device memory would help. However, I don't have a good
understanding of the drm case, hence I could be trying to address a
case that doesn't need to be addressed.

In any case, thanks for your patience and a quick reply.

-- Shuah