| Message ID | 20180611051340.9024-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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);
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(-)