mm: cma: honor __GFP_ZERO flag in cma_alloc()

Message ID 20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2~3rI_9nj8b0455904559eucas1p2C@eucas1p2.samsung.com
State New
Headers show
Series
  • mm: cma: honor __GFP_ZERO flag in cma_alloc()
Related show

Commit Message

Marek Szyprowski June 13, 2018, 8:58 a.m.
cma_alloc() function has gfp mask parameter, so users expect that it
honors typical memory allocation related flags. The most imporant from
the security point of view is handling of __GFP_ZERO flag, because memory
allocated by this function usually can be directly remapped to userspace
by device drivers as a part of multimedia processing and ignoring this
flag might lead to leaking some kernel structures to userspace.
Some callers of this function (for example arm64 dma-iommu glue code)
already assumed that the allocated buffers are cleared when this flag
is set. To avoid such issues, add simple code for clearing newly
allocated buffer when __GFP_ZERO flag is set. Callers will be then
updated to skip implicit clearing or adjust passed gfp flags.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 mm/cma.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.17.1

Comments

Matthew Wilcox June 13, 2018, 12:24 p.m. | #1
On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it

> honors typical memory allocation related flags. The most imporant from

> the security point of view is handling of __GFP_ZERO flag, because memory

> allocated by this function usually can be directly remapped to userspace

> by device drivers as a part of multimedia processing and ignoring this

> flag might lead to leaking some kernel structures to userspace.

> Some callers of this function (for example arm64 dma-iommu glue code)

> already assumed that the allocated buffers are cleared when this flag

> is set. To avoid such issues, add simple code for clearing newly

> allocated buffer when __GFP_ZERO flag is set. Callers will be then

> updated to skip implicit clearing or adjust passed gfp flags.


I think the documentation for this function needs improving.  For example,
GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
At the very least, the kernel-doc needs:

 * Context: Process context (may sleep even if GFP flags indicate otherwise).

Unless someone wants to rework this allocator to use spinlocks instead
of mutexes ...
Marek Szyprowski June 13, 2018, 12:40 p.m. | #2
Hi Matthew,

On 2018-06-13 14:24, Matthew Wilcox wrote:
> On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:

>> cma_alloc() function has gfp mask parameter, so users expect that it

>> honors typical memory allocation related flags. The most imporant from

>> the security point of view is handling of __GFP_ZERO flag, because memory

>> allocated by this function usually can be directly remapped to userspace

>> by device drivers as a part of multimedia processing and ignoring this

>> flag might lead to leaking some kernel structures to userspace.

>> Some callers of this function (for example arm64 dma-iommu glue code)

>> already assumed that the allocated buffers are cleared when this flag

>> is set. To avoid such issues, add simple code for clearing newly

>> allocated buffer when __GFP_ZERO flag is set. Callers will be then

>> updated to skip implicit clearing or adjust passed gfp flags.

> I think the documentation for this function needs improving.  For example,

> GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).

> At the very least, the kernel-doc needs:

>

>   * Context: Process context (may sleep even if GFP flags indicate otherwise).

>

> Unless someone wants to rework this allocator to use spinlocks instead

> of mutexes ...


It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
by the
memory compaction code, which is used in alloc_contig_range(). Right, this
should be also noted in the documentation.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Christoph Hellwig June 13, 2018, 12:52 p.m. | #3
On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it

> honors typical memory allocation related flags. The most imporant from

> the security point of view is handling of __GFP_ZERO flag, because memory

> allocated by this function usually can be directly remapped to userspace

> by device drivers as a part of multimedia processing and ignoring this

> flag might lead to leaking some kernel structures to userspace.

> Some callers of this function (for example arm64 dma-iommu glue code)

> already assumed that the allocated buffers are cleared when this flag

> is set. To avoid such issues, add simple code for clearing newly

> allocated buffer when __GFP_ZERO flag is set. Callers will be then

> updated to skip implicit clearing or adjust passed gfp flags.


dma mapping implementations need to zero all memory returned anyway
(even if a few implementation don't do that yet).

I'd rather keep the zeroing in the common callers.
Christoph Hellwig June 13, 2018, 12:55 p.m. | #4
On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 

> by the

> memory compaction code, which is used in alloc_contig_range(). Right, this

> should be also noted in the documentation.


Documentation is good, asserts are better.  The code should reject any
flag not explicitly supported, or even better have its own flags type
with the few actually supported flags.
Michal Hocko June 13, 2018, 1:39 p.m. | #5
On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:

> > It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 

> > by the

> > memory compaction code, which is used in alloc_contig_range(). Right, this

> > should be also noted in the documentation.

> 

> Documentation is good, asserts are better.  The code should reject any

> flag not explicitly supported, or even better have its own flags type

> with the few actually supported flags.


Agreed. Is the cma allocator used for anything other than GFP_KERNEL
btw.? If not then, shouldn't we simply drop the gfp argument altogether
rather than give users a false hope for differen gfp modes that are not
really supported and grow broken code?

-- 
Michal Hocko
SUSE Labs

Patch

diff --git a/mm/cma.c b/mm/cma.c
index 5809bbe360d7..1106d5aef2cc 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -464,6 +464,13 @@  struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 		start = bitmap_no + mask + 1;
 	}
 
+	if (ret == 0 && gfp_mask & __GFP_ZERO) {
+		int i;
+
+		for (i = 0; i < count; i++)
+			clear_highpage(page + i);
+	}
+
 	trace_cma_alloc(pfn, page, count, align);
 
 	if (ret && !(gfp_mask & __GFP_NOWARN)) {