[U-Boot] [PATCH] linux_compat: fix potential NULL pointer access

[U-Boot] [PATCH] linux_compat: fix potential NULL pointer access

[Marek Szyprowski]

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

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

[U-Boot] [PATCH] linux_compat: fix potential NULL pointer access

[Ralph Siemsen]

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

[U-Boot] [PATCH] linux_compat: fix potential NULL pointer access

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191101/c14d0a28/attachment.sig>