[PATCH] arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

[PATCH] arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

[Marek Szyprowski]

dma_alloc_*() buffers might be exposed to userspace via mmap() call, so
they should be cleared on allocation. In case of IOMMU-based dma-mapping
implementation such buffer clearing was missing in the code path for
DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
more information on clearing buffers allocated by dma_alloc_* functions,
see commit 44176bb38fa4 ("arm64: dma-mapping: always clear allocated
buffers").

Fixes: 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm64/mm/dma-mapping.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 632d32109755..aa0037a3185f 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -629,13 +629,14 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 						    size >> PAGE_SHIFT);
 			return NULL;
 		}
-		if (!coherent)
-			__dma_flush_area(page_to_virt(page), iosize);
-
 		addr = dma_common_contiguous_remap(page, size, VM_USERMAP,
 						   prot,
 						   __builtin_return_address(0));
-		if (!addr) {
+		if (addr) {
+			memset(addr, 0, size);
+			if (!coherent)
+				__dma_flush_area(page_to_virt(page), iosize);
+		} else {
 			iommu_dma_unmap_page(dev, *handle, iosize, 0, attrs);
 			dma_release_from_contiguous(dev, page,
 						    size >> PAGE_SHIFT);
-- 
2.17.1

[PATCH] arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

[Geert Uytterhoeven]

Hi Marek,

Thanks for your patch!

On Mon, Jun 11, 2018 at 7:14 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> dma_alloc_*() buffers might be exposed to userspace via mmap() call, so
> they should be cleared on allocation. In case of IOMMU-based dma-mapping
> implementation such buffer clearing was missing in the code path for
> DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For

Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp),
and __iommu_alloc_attrs() has

        /*
         * Some drivers rely on this, and we probably don't want the
         * possibility of stale kernel data being read by devices anyway.
         */
        gfp |= __GFP_ZERO;

at the top, before the allocation.

If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor
__GFP_ZERO, I think cma_alloc() should be fixed instead.

> more information on clearing buffers allocated by dma_alloc_* functions,
> see commit 44176bb38fa4 ("arm64: dma-mapping: always clear allocated

6829e274a623

> buffers").
>
> Fixes: 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

[Will Deacon]

On Mon, Jun 11, 2018 at 09:55:54AM +0200, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> Thanks for your patch!
> 
> On Mon, Jun 11, 2018 at 7:14 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> > dma_alloc_*() buffers might be exposed to userspace via mmap() call, so
> > they should be cleared on allocation. In case of IOMMU-based dma-mapping
> > implementation such buffer clearing was missing in the code path for
> > DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
> 
> Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp),
> and __iommu_alloc_attrs() has
> 
>         /*
>          * Some drivers rely on this, and we probably don't want the
>          * possibility of stale kernel data being read by devices anyway.
>          */
>         gfp |= __GFP_ZERO;
> 
> at the top, before the allocation.
> 
> If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor
> __GFP_ZERO, I think cma_alloc() should be fixed instead.

Agreed. We tried to fix this in 7132813c3845 ("arm64: Honor __GFP_ZERO in
dma allocations"). Has something broken that?

Will

[PATCH] arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

[Marek Szyprowski]

On 2018-06-11 14:31, Will Deacon wrote:
> On Mon, Jun 11, 2018 at 09:55:54AM +0200, Geert Uytterhoeven wrote:
>> Hi Marek,
>>
>> Thanks for your patch!
>>
>> On Mon, Jun 11, 2018 at 7:14 AM Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>> dma_alloc_*() buffers might be exposed to userspace via mmap() call, so
>>> they should be cleared on allocation. In case of IOMMU-based dma-mapping
>>> implementation such buffer clearing was missing in the code path for
>>> DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
>> Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp),
>> and __iommu_alloc_attrs() has
>>
>>          /*
>>           * Some drivers rely on this, and we probably don't want the
>>           * possibility of stale kernel data being read by devices anyway.
>>           */
>>          gfp |= __GFP_ZERO;
>>
>> at the top, before the allocation.
>>
>> If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor
>> __GFP_ZERO, I think cma_alloc() should be fixed instead.
> Agreed. We tried to fix this in 7132813c3845 ("arm64: Honor __GFP_ZERO in
> dma allocations"). Has something broken that?

The code for handling DMA_ATTR_FORCE_CONTIGUOUS flag in arm64 
dma-mapping/iommu
implementation has been added later and assumed that __GFP_ZERO flag is
handled by dma_alloc_from_contiguous(). This is sadly not true, so the 
buffer
allocated this way is not cleared.

In case of ARM (32bit) the newly allocated buffer is always cleared directly
after calling dma_alloc_from_contiguous() function.

I agree that adding handling of __GFP_ZERO flag to 
dma_alloc_from_contiguous()
or rather cma_alloc() is a better idea, but might have some side effects, so
such change is not a good candidate for -rc cycle. I will prepare such 
patch,
but for now this one is imho a bit less invasive approach.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH] arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

[Geert Uytterhoeven]

Hi Marek,

On Tue, Jun 12, 2018 at 9:31 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 2018-06-11 14:31, Will Deacon wrote:
> > On Mon, Jun 11, 2018 at 09:55:54AM +0200, Geert Uytterhoeven wrote:
> >> On Mon, Jun 11, 2018 at 7:14 AM Marek Szyprowski
> >> <m.szyprowski@samsung.com> wrote:
> >>> dma_alloc_*() buffers might be exposed to userspace via mmap() call, so
> >>> they should be cleared on allocation. In case of IOMMU-based dma-mapping
> >>> implementation such buffer clearing was missing in the code path for
> >>> DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
> >> Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp),
> >> and __iommu_alloc_attrs() has
> >>
> >>          /*
> >>           * Some drivers rely on this, and we probably don't want the
> >>           * possibility of stale kernel data being read by devices anyway.
> >>           */
> >>          gfp |= __GFP_ZERO;
> >>
> >> at the top, before the allocation.
> >>
> >> If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor
> >> __GFP_ZERO, I think cma_alloc() should be fixed instead.
> > Agreed. We tried to fix this in 7132813c3845 ("arm64: Honor __GFP_ZERO in
> > dma allocations"). Has something broken that?
>
> The code for handling DMA_ATTR_FORCE_CONTIGUOUS flag in arm64
> dma-mapping/iommu
> implementation has been added later and assumed that __GFP_ZERO flag is
> handled by dma_alloc_from_contiguous(). This is sadly not true, so the
> buffer
> allocated this way is not cleared.
>
> In case of ARM (32bit) the newly allocated buffer is always cleared directly
> after calling dma_alloc_from_contiguous() function.

JFTR, mips and generic dma_direct_alloc() also do that, extensa does not.

> I agree that adding handling of __GFP_ZERO flag to
> dma_alloc_from_contiguous()
> or rather cma_alloc() is a better idea, but might have some side effects, so
> such change is not a good candidate for -rc cycle. I will prepare such
> patch,
> but for now this one is imho a bit less invasive approach.

OK, fine for me.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds