[PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Marek Szyprowski]

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(+)

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)) {
-- 
2.17.1

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Matthew Wilcox]

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 ...

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Marek Szyprowski]

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

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Christoph Hellwig]

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.

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Michal Hocko]

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

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Marek Szyprowski]

Hi Michal,

On 2018-06-13 15:39, Michal Hocko wrote:
> 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?

Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp 
mask.
The only flag which is now checked is __GFP_NOWARN. I can change the 
function
signature of cma_alloc to:
struct page *cma_alloc(struct cma *cma, size_t count, unsigned int 
align, bool no_warn);

What about clearing the allocated buffer? Should it be another bool 
parameter,
done unconditionally or moved to the callers?

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

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Christoph Hellwig]

On Mon, Jul 02, 2018 at 03:23:34PM +0200, Marek Szyprowski wrote:
> What about clearing the allocated buffer? Should it be another bool 
> parameter,
> done unconditionally or moved to the callers?

Please keep it in the callers.  I plan to push it up even higher
from the current callers short to midterm.

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Marek Szyprowski]

Hi Christoph,

On 2018-07-02 15:30, Christoph Hellwig wrote:
> On Mon, Jul 02, 2018 at 03:23:34PM +0200, Marek Szyprowski wrote:
>> What about clearing the allocated buffer? Should it be another bool
>> parameter,
>> done unconditionally or moved to the callers?
> Please keep it in the callers.  I plan to push it up even higher
> from the current callers short to midterm.

Okay, I will post a patch with this approach then.

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

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Michal Hocko]

On Mon 02-07-18 15:23:34, Marek Szyprowski wrote:
> Hi Michal,
> 
> On 2018-06-13 15:39, Michal Hocko wrote:
> > 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?
> 
> Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp 
> mask.
> The only flag which is now checked is __GFP_NOWARN. I can change the 
> function
> signature of cma_alloc to:
> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int 
> align, bool no_warn);

Are there any __GFP_NOWARN users? I have quickly hit the indirection
trap and searching for alloc callback didn't tell me really much.

> What about clearing the allocated buffer? Should it be another bool
> parameter, done unconditionally or moved to the callers?

That really depends on callers. I have no idea what they actually ask
for.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Marek Szyprowski]

Hi Michal,

On 2018-07-02 15:32, Michal Hocko wrote:
> On Mon 02-07-18 15:23:34, Marek Szyprowski wrote:
>> On 2018-06-13 15:39, Michal Hocko wrote:
>>> 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?
>> Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp
>> mask.
>> The only flag which is now checked is __GFP_NOWARN. I can change the
>> function
>> signature of cma_alloc to:
>> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int
>> align, bool no_warn);
> Are there any __GFP_NOWARN users? I have quickly hit the indirection
> trap and searching for alloc callback didn't tell me really much.

They might be via dma_alloc_from_contiguous() and dma_alloc_*() path.

>> What about clearing the allocated buffer? Should it be another bool
>> parameter, done unconditionally or moved to the callers?
> That really depends on callers. I have no idea what they actually ask
> for.

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

Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()

[Christoph Hellwig]

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.