[U-Boot] linux_compat: fix potential NULL pointer access
diff mbox series

Message ID 20191002123720.22546-1-m.szyprowski@samsung.com
State Accepted
Commit 2ad98ab8f68c71e2db140c6d5f1020aa6fbacb9e
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot] linux_compat: fix potential NULL pointer access
Related show

Commit Message

Marek Szyprowski Oct. 2, 2019, 12:37 p.m. UTC
malloc_cache_aligned() might return zero, so fix potential NULL pointer
access if __GFP_ZERO flag is set.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 lib/linux_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ralph Siemsen Oct. 3, 2019, 2:23 p.m. UTC | #1
On Wed, Oct 02, 2019 at 02:37:20PM +0200, Marek Szyprowski wrote:
>malloc_cache_aligned() might return zero, so fix potential NULL pointer
>access if __GFP_ZERO flag is set.
>
>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Ralph Siemsen <ralph.siemsen@linaro.org>

This looks reasonable to me. The memset() will happily scribble all over 
memory at address zero, which can be very hard to track down later on.
So adding a check seems prudent. Note that I am not a maintainer ;-)

>---
> lib/linux_compat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/lib/linux_compat.c b/lib/linux_compat.c
>index 6373b4451e..81ea8fb126 100644
>--- a/lib/linux_compat.c
>+++ b/lib/linux_compat.c
>@@ -20,7 +20,7 @@ void *kmalloc(size_t size, int flags)
> 	void *p;
>
> 	p = malloc_cache_aligned(size);
>-	if (flags & __GFP_ZERO)
>+	if (p && flags & __GFP_ZERO)
> 		memset(p, 0, size);
>
> 	return p;
>-- 
>2.17.1

Perhaps I can hijack this thread slightly: have others encountered 
problems due to the use of malloc_cache_aligned() in kmalloc() and 
kmem_cache_alloc()? This substantially increases memory usage, in 
particular for UBI which allocates two small structures for every 
physical erase block. This adds up quickly on large flash chips.

I understand the rationale for padding the allocation, as explained in 
commit e3332e1a1a04534225801c2710c6faef4809641c. However, it also seems 
that in Linux, allocations are aligned/padded only when certain flags 
are set (GFP_DMA or GFP_DMA32 in particular). These flags are not 
currently defined in u-boot.

For UBI at least, there is no need to align or pad the structures, they 
are not accessed by DMA. So it would be good if we could avoid memory 
overhead. Possible options would include:

- go back to plain memalign() or even just plain malloc(),
  with the caveat that callers who need specific alignment must
  handle it themselves (simplest option, no overhead)
- add the missing flags, and check them at runtime.
  (adding code and some overhead)
- hybrid versions, such as checking flags on kmalloc, but not doing
  any padding/alignment for kmem_cache_alloc

I would be happy to work on patches for this, in a separate thread of 
course. Would be helpful to know which option would be acceptable.

Cheers,
Ralph
Tom Rini Nov. 1, 2019, 1:29 p.m. UTC | #2
On Wed, Oct 02, 2019 at 02:37:20PM +0200, Marek Szyprowski wrote:

> malloc_cache_aligned() might return zero, so fix potential NULL pointer
> access if __GFP_ZERO flag is set.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Ralph Siemsen <ralph.siemsen@linaro.org>

Applied to u-boot/master, thanks!

Patch
diff mbox series

diff --git a/lib/linux_compat.c b/lib/linux_compat.c
index 6373b4451e..81ea8fb126 100644
--- a/lib/linux_compat.c
+++ b/lib/linux_compat.c
@@ -20,7 +20,7 @@  void *kmalloc(size_t size, int flags)
 	void *p;
 
 	p = malloc_cache_aligned(size);
-	if (flags & __GFP_ZERO)
+	if (p && flags & __GFP_ZERO)
 		memset(p, 0, size);
 
 	return p;