| Message ID | 20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2~3rI_9nj8b0455904559eucas1p2C@eucas1p2.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ...
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
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.
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.
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?
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
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.
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.
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
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
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)) {
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(+)