[U-Boot] [PATCH V2 1/2] malloc: use hidden visibility
When running sandbox, the following phases occur, each with different malloc implementations or behaviors:
1) Dynamic linker execution, using the dynamic linker's own malloc() implementation. This is fully functional.
2) After U-Boot's malloc symbol has been hooked into the GOT, but before any U-Boot code has run. This phase is entirely non-functional, since U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and mem_malloc_init() have not been called.
At least on Ubuntu Xenial, the dynamic linker does make both malloc() and free() calls during this phase. Currently these free() calls crash since they dereference gd, which is NULL.
U-Boot itself makes no use of malloc() during this phase.
3) U-Boot execution after gd is set and initf_malloc() has been called. This is fully functional, albeit via a very simple malloc() implementation.
4) U-Boot execution after mem_malloc_init() has been called. This is fully functional with a complete malloc() implementation.
Furthermore, if code that called malloc() during phase 1 calls free() in phase 3 or later, it is likely that heap corruption will occur, since U-Boot's malloc implementation will assume the pointer is part of its own heap, although it isn't. I have not actively observed this happening.
To prevent phase 2 from happening, this patch makes all of U-Boot's malloc library public symbols have hidden visibility. This prevents them from being hooked into the GOT, so only code in the U-Boot binary itself actually calls them; any other code will call into the standard C library malloc(). This also avoids the "furthermore" issue mentioned above.
I have seen references to this GCC pragma in blog posts from 2008, and RHEL5's ancient gcc appears to accept it fine, so I believe it's quite safe to use it without checking gcc version.
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org --- v2: A whole different approach. --- include/malloc.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/malloc.h b/include/malloc.h index f20e4d3d2a6b..8175c75920cf 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -914,6 +914,7 @@ int initf_malloc(void); /* Simple versions which can be used when space is tight */ void *malloc_simple(size_t size);
+#pragma GCC visibility push(hidden) # if __STD_C
Void_t* mALLOc(size_t); @@ -945,6 +946,7 @@ int mALLOPt(); struct mallinfo mALLINFo(); # endif #endif +#pragma GCC visibility pop
/* * Begin and End of memory area for malloc(), and current "brk"
Following the previous patch, malloc() is never called before gd is set, so we can remove the special-case check for this condition.
This reverts commit 854d2b9753e4 "dlmalloc: ensure gd is set for early alloc".
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org --- common/dlmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 5ea37dfb6e4c..d66e80c647ce 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -2184,7 +2184,7 @@ Void_t* mALLOc(bytes) size_t bytes; INTERNAL_SIZE_T nb;
#ifdef CONFIG_SYS_MALLOC_F_LEN - if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) + if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) return malloc_simple(bytes); #endif
On Sat, Mar 05, 2016 at 10:30:53AM -0700, Stephen Warren wrote:
Following the previous patch, malloc() is never called before gd is set, so we can remove the special-case check for this condition.
This reverts commit 854d2b9753e4 "dlmalloc: ensure gd is set for early alloc".
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org
Reviewed-by: Tom Rini trini@konsulko.com
On 5 March 2016 at 10:30, Stephen Warren swarren@wwwdotorg.org wrote:
Following the previous patch, malloc() is never called before gd is set, so we can remove the special-case check for this condition.
This reverts commit 854d2b9753e4 "dlmalloc: ensure gd is set for early alloc".
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org
common/dlmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, Mar 05, 2016 at 10:30:53AM -0700, Stephen Warren wrote:
Following the previous patch, malloc() is never called before gd is set, so we can remove the special-case check for this condition.
This reverts commit 854d2b9753e4 "dlmalloc: ensure gd is set for early alloc".
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
On Sat, Mar 05, 2016 at 10:30:52AM -0700, Stephen Warren wrote:
When running sandbox, the following phases occur, each with different malloc implementations or behaviors:
- Dynamic linker execution, using the dynamic linker's own malloc()
implementation. This is fully functional.
- After U-Boot's malloc symbol has been hooked into the GOT, but before
any U-Boot code has run. This phase is entirely non-functional, since U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and mem_malloc_init() have not been called.
At least on Ubuntu Xenial, the dynamic linker does make both malloc() and free() calls during this phase. Currently these free() calls crash since they dereference gd, which is NULL.
U-Boot itself makes no use of malloc() during this phase.
- U-Boot execution after gd is set and initf_malloc() has been called.
This is fully functional, albeit via a very simple malloc() implementation.
- U-Boot execution after mem_malloc_init() has been called. This is fully
functional with a complete malloc() implementation.
Furthermore, if code that called malloc() during phase 1 calls free() in phase 3 or later, it is likely that heap corruption will occur, since U-Boot's malloc implementation will assume the pointer is part of its own heap, although it isn't. I have not actively observed this happening.
To prevent phase 2 from happening, this patch makes all of U-Boot's malloc library public symbols have hidden visibility. This prevents them from being hooked into the GOT, so only code in the U-Boot binary itself actually calls them; any other code will call into the standard C library malloc(). This also avoids the "furthermore" issue mentioned above.
I have seen references to this GCC pragma in blog posts from 2008, and RHEL5's ancient gcc appears to accept it fine, so I believe it's quite safe to use it without checking gcc version.
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org
Reviewed-by: Tom Rini trini@konsulko.com
But have you tried with clang instead of gcc? I suspect it's got code to pretend to be gcc and do the right thing here but it's worth confirming.
On 03/05/2016 10:44 AM, Tom Rini wrote:
On Sat, Mar 05, 2016 at 10:30:52AM -0700, Stephen Warren wrote:
When running sandbox, the following phases occur, each with different malloc implementations or behaviors:
- Dynamic linker execution, using the dynamic linker's own malloc()
implementation. This is fully functional.
- After U-Boot's malloc symbol has been hooked into the GOT, but before
any U-Boot code has run. This phase is entirely non-functional, since U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and mem_malloc_init() have not been called.
At least on Ubuntu Xenial, the dynamic linker does make both malloc() and free() calls during this phase. Currently these free() calls crash since they dereference gd, which is NULL.
U-Boot itself makes no use of malloc() during this phase.
- U-Boot execution after gd is set and initf_malloc() has been called.
This is fully functional, albeit via a very simple malloc() implementation.
- U-Boot execution after mem_malloc_init() has been called. This is fully
functional with a complete malloc() implementation.
Furthermore, if code that called malloc() during phase 1 calls free() in phase 3 or later, it is likely that heap corruption will occur, since U-Boot's malloc implementation will assume the pointer is part of its own heap, although it isn't. I have not actively observed this happening.
To prevent phase 2 from happening, this patch makes all of U-Boot's malloc library public symbols have hidden visibility. This prevents them from being hooked into the GOT, so only code in the U-Boot binary itself actually calls them; any other code will call into the standard C library malloc(). This also avoids the "furthermore" issue mentioned above.
I have seen references to this GCC pragma in blog posts from 2008, and RHEL5's ancient gcc appears to accept it fine, so I believe it's quite safe to use it without checking gcc version.
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org
Reviewed-by: Tom Rini trini@konsulko.com
But have you tried with clang instead of gcc? I suspect it's got code to pretend to be gcc and do the right thing here but it's worth confirming.
Well, clang for sandbox doesn't compile for reasons other than these patches on either Ubuntu 14.04 or 16.04 pre-release, nor using the instructions in doc/README.clang for rpi on 16.04. However, it did work for rpi on Ubuntu 14.04.
So in summary, I think there's no issue using this pragma with clang, but I can't test that clang actually implements it and hence solves the problem these patches fix.
On Sat, Mar 05, 2016 at 11:31:46AM -0700, Stephen Warren wrote:
On 03/05/2016 10:44 AM, Tom Rini wrote:
On Sat, Mar 05, 2016 at 10:30:52AM -0700, Stephen Warren wrote:
When running sandbox, the following phases occur, each with different malloc implementations or behaviors:
- Dynamic linker execution, using the dynamic linker's own malloc()
implementation. This is fully functional.
- After U-Boot's malloc symbol has been hooked into the GOT, but before
any U-Boot code has run. This phase is entirely non-functional, since U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and mem_malloc_init() have not been called.
At least on Ubuntu Xenial, the dynamic linker does make both malloc() and free() calls during this phase. Currently these free() calls crash since they dereference gd, which is NULL.
U-Boot itself makes no use of malloc() during this phase.
- U-Boot execution after gd is set and initf_malloc() has been called.
This is fully functional, albeit via a very simple malloc() implementation.
- U-Boot execution after mem_malloc_init() has been called. This is fully
functional with a complete malloc() implementation.
Furthermore, if code that called malloc() during phase 1 calls free() in phase 3 or later, it is likely that heap corruption will occur, since U-Boot's malloc implementation will assume the pointer is part of its own heap, although it isn't. I have not actively observed this happening.
To prevent phase 2 from happening, this patch makes all of U-Boot's malloc library public symbols have hidden visibility. This prevents them from being hooked into the GOT, so only code in the U-Boot binary itself actually calls them; any other code will call into the standard C library malloc(). This also avoids the "furthermore" issue mentioned above.
I have seen references to this GCC pragma in blog posts from 2008, and RHEL5's ancient gcc appears to accept it fine, so I believe it's quite safe to use it without checking gcc version.
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org
Reviewed-by: Tom Rini trini@konsulko.com
But have you tried with clang instead of gcc? I suspect it's got code to pretend to be gcc and do the right thing here but it's worth confirming.
Well, clang for sandbox doesn't compile for reasons other than these patches on either Ubuntu 14.04 or 16.04 pre-release, nor using the instructions in doc/README.clang for rpi on 16.04. However, it did work for rpi on Ubuntu 14.04.
So in summary, I think there's no issue using this pragma with clang, but I can't test that clang actually implements it and hence solves the problem these patches fix.
OK, I did a quick local check after fixing the __BIGGEST_ALIGNMENT__ problem (but not the linking problem) and see llvm 3.5 at least is OK with this, so we're good enough then, thanks!
On 5 March 2016 at 10:30, Stephen Warren swarren@wwwdotorg.org wrote:
When running sandbox, the following phases occur, each with different malloc implementations or behaviors:
- Dynamic linker execution, using the dynamic linker's own malloc()
implementation. This is fully functional.
- After U-Boot's malloc symbol has been hooked into the GOT, but before
any U-Boot code has run. This phase is entirely non-functional, since U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and mem_malloc_init() have not been called.
At least on Ubuntu Xenial, the dynamic linker does make both malloc() and free() calls during this phase. Currently these free() calls crash since they dereference gd, which is NULL.
U-Boot itself makes no use of malloc() during this phase.
- U-Boot execution after gd is set and initf_malloc() has been called.
This is fully functional, albeit via a very simple malloc() implementation.
- U-Boot execution after mem_malloc_init() has been called. This is fully
functional with a complete malloc() implementation.
Furthermore, if code that called malloc() during phase 1 calls free() in phase 3 or later, it is likely that heap corruption will occur, since U-Boot's malloc implementation will assume the pointer is part of its own heap, although it isn't. I have not actively observed this happening.
To prevent phase 2 from happening, this patch makes all of U-Boot's malloc library public symbols have hidden visibility. This prevents them from being hooked into the GOT, so only code in the U-Boot binary itself actually calls them; any other code will call into the standard C library malloc(). This also avoids the "furthermore" issue mentioned above.
I have seen references to this GCC pragma in blog posts from 2008, and RHEL5's ancient gcc appears to accept it fine, so I believe it's quite safe to use it without checking gcc version.
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org
v2: A whole different approach.
include/malloc.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/malloc.h b/include/malloc.h index f20e4d3d2a6b..8175c75920cf 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -914,6 +914,7 @@ int initf_malloc(void); /* Simple versions which can be used when space is tight */ void *malloc_simple(size_t size);
+#pragma GCC visibility push(hidden) # if __STD_C
Void_t* mALLOc(size_t); @@ -945,6 +946,7 @@ int mALLOPt(); struct mallinfo mALLINFo(); # endif #endif +#pragma GCC visibility pop
/*
- Begin and End of memory area for malloc(), and current "brk"
-- 2.7.0
This seems better than what we have. Thanks for digging into it.
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, Mar 05, 2016 at 10:30:52AM -0700, Stephen Warren wrote:
When running sandbox, the following phases occur, each with different malloc implementations or behaviors:
- Dynamic linker execution, using the dynamic linker's own malloc()
implementation. This is fully functional.
- After U-Boot's malloc symbol has been hooked into the GOT, but before
any U-Boot code has run. This phase is entirely non-functional, since U-Boot's gd symbol is NULL and U-Boot's initf_malloc() and mem_malloc_init() have not been called.
At least on Ubuntu Xenial, the dynamic linker does make both malloc() and free() calls during this phase. Currently these free() calls crash since they dereference gd, which is NULL.
U-Boot itself makes no use of malloc() during this phase.
- U-Boot execution after gd is set and initf_malloc() has been called.
This is fully functional, albeit via a very simple malloc() implementation.
- U-Boot execution after mem_malloc_init() has been called. This is fully
functional with a complete malloc() implementation.
Furthermore, if code that called malloc() during phase 1 calls free() in phase 3 or later, it is likely that heap corruption will occur, since U-Boot's malloc implementation will assume the pointer is part of its own heap, although it isn't. I have not actively observed this happening.
To prevent phase 2 from happening, this patch makes all of U-Boot's malloc library public symbols have hidden visibility. This prevents them from being hooked into the GOT, so only code in the U-Boot binary itself actually calls them; any other code will call into the standard C library malloc(). This also avoids the "furthermore" issue mentioned above.
I have seen references to this GCC pragma in blog posts from 2008, and RHEL5's ancient gcc appears to accept it fine, so I believe it's quite safe to use it without checking gcc version.
Cc: Rabin Vincent rabin@rab.in Signed-off-by: Stephen Warren swarren@wwwdotorg.org Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (3)
-
Simon Glass -
Stephen Warren -
Tom Rini