mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH master 1/2] test: self: add basic testing for malloc()
@ 2022-05-23  6:27 Ahmad Fatoum
  2022-05-23  6:27 ` [PATCH master 2/2] tlsf: fix internal overflow trying to allocate big buffers Ahmad Fatoum
  2022-05-24  7:04 ` [PATCH master 1/2] test: self: add basic testing for malloc() Sascha Hauer
  0 siblings, 2 replies; 3+ messages in thread
From: Ahmad Fatoum @ 2022-05-23  6:27 UTC (permalink / raw)
  To: barebox; +Cc: j.martin, Ahmad Fatoum

Surprisingly, the TLSF allocator is prone to overflow. Add some tests
that trigger this behavior. These tests run to success with dlmalloc,
but some of them fail under tlsf when compiled for 32-bit:

  test_malloc:40: unexpectedly succeeded to malloc(SIZE_MAX)
  test_malloc:61: unexpectedly succeeded to (tmp = realloc(p, 0xf0000000))
  test_malloc:63: unexpectedly succeeded to tmp = realloc(p, SIZE_MAX)
  ERROR: malloc: failed 3 out of 12 tests

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 test/self/Kconfig  |  5 +++
 test/self/Makefile |  1 +
 test/self/malloc.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+)
 create mode 100644 test/self/malloc.c

diff --git a/test/self/Kconfig b/test/self/Kconfig
index 3340a9146ee2..cf11efe54486 100644
--- a/test/self/Kconfig
+++ b/test/self/Kconfig
@@ -32,6 +32,11 @@ config SELFTEST_ENABLE_ALL
 	help
 	  Selects all self-tests compatible with current configuration
 
+config SELFTEST_MALLOC
+	bool "malloc() selftest"
+	help
+	  Tests barebox memory allocator
+
 config SELFTEST_PRINTF
 	bool "printf selftest"
 	help
diff --git a/test/self/Makefile b/test/self/Makefile
index 05a2a6a236ec..65d01596b806 100644
--- a/test/self/Makefile
+++ b/test/self/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_SELFTEST) += core.o
+obj-$(CONFIG_SELFTEST_MALLOC) += malloc.o
 obj-$(CONFIG_SELFTEST_PRINTF) += printf.o
 obj-$(CONFIG_SELFTEST_PROGRESS_NOTIFIER) += progress-notifier.o
 obj-$(CONFIG_SELFTEST_OF_MANIPULATION) += of_manipulation.o of_manipulation.dtb.o
diff --git a/test/self/malloc.c b/test/self/malloc.c
new file mode 100644
index 000000000000..c25b416b9751
--- /dev/null
+++ b/test/self/malloc.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <common.h>
+#include <bselftest.h>
+#include <malloc.h>
+#include <memory.h>
+#include <linux/sizes.h>
+
+BSELFTEST_GLOBALS();
+
+static void __expect(bool cond, bool expect,
+		     const char *condstr, const char *func, int line)
+{
+	total_tests++;
+	if (cond != expect) {
+		failed_tests++;
+		printf("%s:%d: %s to %s\n", func, line,
+		       expect ? "failed" : "unexpectedly succeeded",
+		       condstr);
+	}
+}
+
+#define expect_alloc_ok(cond) \
+	__expect((cond), true, #cond, __func__, __LINE__)
+
+#define expect_alloc_fail(cond) \
+	__expect((cond), false, #cond, __func__, __LINE__)
+
+static void test_malloc(void)
+{
+	size_t mem_malloc_size = mem_malloc_end() - mem_malloc_start();
+	u8 *p, *tmp;
+
+	pr_debug("mem_malloc_size = %zu\n", mem_malloc_size);
+
+	/* System libc when built for sandbox may have overcommit, so
+	 * doing very big allocations without actual use may succeed
+	 * unlike in-barebox allocators, so skip these tests in that
+	 * case
+	 */
+	if (IS_ENABLED(CONFIG_MALLOC_LIBC)) {
+		pr_info("built with host libc allocator: Skipping tests that may trigger overcommit\n");
+		mem_malloc_size = 0;
+	}
+
+	expect_alloc_ok(p = malloc(1));
+	free(p);
+
+	if (mem_malloc_size) {
+		expect_alloc_fail(malloc(SIZE_MAX));
+
+		if (0xf0000000 > mem_malloc_size) {
+			expect_alloc_fail((tmp = malloc(0xf0000000)));
+			free(tmp);
+		}
+	} else {
+		skipped_tests += 2;
+	}
+
+	p = realloc(NULL, 1);
+	expect_alloc_ok(p = realloc(NULL, 1));
+
+	*p = 0x42;
+
+	expect_alloc_ok(tmp = realloc(p, 2));
+
+	p = tmp;
+	__expect(*p == 0x42, true, "reread after realloc", __func__, __LINE__);
+
+	if (mem_malloc_size) {
+		expect_alloc_fail(tmp = realloc(p, mem_malloc_size));
+
+		if (0xf0000000 > mem_malloc_size)
+			expect_alloc_fail((tmp = realloc(p, 0xf0000000)));
+
+		expect_alloc_fail(tmp = realloc(p, SIZE_MAX));
+
+	} else {
+		skipped_tests += 3;
+	}
+
+	free(p);
+
+	expect_alloc_ok(p = malloc(0));
+	expect_alloc_ok(tmp = malloc(0));
+
+	__expect(p != tmp, true, "allocate distinct 0-size buffers", __func__, __LINE__);
+}
+bselftest(core, test_malloc);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH master 2/2] tlsf: fix internal overflow trying to allocate big buffers
  2022-05-23  6:27 [PATCH master 1/2] test: self: add basic testing for malloc() Ahmad Fatoum
@ 2022-05-23  6:27 ` Ahmad Fatoum
  2022-05-24  7:04 ` [PATCH master 1/2] test: self: add basic testing for malloc() Sascha Hauer
  1 sibling, 0 replies; 3+ messages in thread
From: Ahmad Fatoum @ 2022-05-23  6:27 UTC (permalink / raw)
  To: barebox; +Cc: j.martin, Ahmad Fatoum

The function adjust_request_size() has an unhandled failure mode:
If aligning a buffer up overflows SIZE_MAX, it will compute a way to
short buffer instead of propagating an error. Fix this by returning
0 in this case and checking for 0 whereever the function is called.

0 is a safe choice for an error code, because the function returns
at least block_size_min on success and 0 was already an error code
(that was just never handled).

Reported-by: Jonas Martin <j.martin@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/tlsf.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/common/tlsf.c b/common/tlsf.c
index 520cce496ef6..3ca58e3abbfb 100644
--- a/common/tlsf.c
+++ b/common/tlsf.c
@@ -313,7 +313,7 @@ static size_t adjust_request_size(size_t size, size_t align)
 		const size_t aligned = align_up(size, align);
 
 		/* aligned sized must not exceed block_size_max or we'll go out of bounds on sl_bitmap */
-		if (aligned < block_size_max)
+		if (aligned >= size && aligned < block_size_max)
 		{
 			adjust = tlsf_max(aligned, block_size_min);
 		}
@@ -942,7 +942,12 @@ void* tlsf_malloc(tlsf_t tlsf, size_t size)
 {
 	control_t* control = tlsf_cast(control_t*, tlsf);
 	const size_t adjust = adjust_request_size(size, ALIGN_SIZE);
-	block_header_t* block = block_locate_free(control, adjust);
+	block_header_t* block;
+
+	if (!adjust)
+		return NULL;
+
+	block = block_locate_free(control, adjust);
 
 	return block_prepare_used(control, block, adjust, size);
 }
@@ -969,7 +974,12 @@ void* tlsf_memalign(tlsf_t tlsf, size_t align, size_t size)
 	*/
 	const size_t aligned_size = (adjust && align > ALIGN_SIZE) ? size_with_gap : adjust;
 
-	block_header_t* block = block_locate_free(control, aligned_size);
+	block_header_t* block;
+
+	if (!adjust || !size_with_gap)
+		return NULL;
+
+	block = block_locate_free(control, aligned_size);
 
 	/* This can't be a static assert. */
 	tlsf_assert(sizeof(block_header_t) == block_size_min + block_header_overhead);
@@ -1059,6 +1069,9 @@ void* tlsf_realloc(tlsf_t tlsf, void* ptr, size_t size)
 
 		tlsf_assert(!block_is_free(block) && "block already marked as free");
 
+		if (!adjust)
+			return NULL;
+
 		/*
 		** If the next block is used, or when combined with the current
 		** block, does not offer enough space, we must reallocate and copy.
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH master 1/2] test: self: add basic testing for malloc()
  2022-05-23  6:27 [PATCH master 1/2] test: self: add basic testing for malloc() Ahmad Fatoum
  2022-05-23  6:27 ` [PATCH master 2/2] tlsf: fix internal overflow trying to allocate big buffers Ahmad Fatoum
@ 2022-05-24  7:04 ` Sascha Hauer
  1 sibling, 0 replies; 3+ messages in thread
From: Sascha Hauer @ 2022-05-24  7:04 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, j.martin

On Mon, May 23, 2022 at 08:27:55AM +0200, Ahmad Fatoum wrote:
> Surprisingly, the TLSF allocator is prone to overflow. Add some tests
> that trigger this behavior. These tests run to success with dlmalloc,
> but some of them fail under tlsf when compiled for 32-bit:
> 
>   test_malloc:40: unexpectedly succeeded to malloc(SIZE_MAX)
>   test_malloc:61: unexpectedly succeeded to (tmp = realloc(p, 0xf0000000))
>   test_malloc:63: unexpectedly succeeded to tmp = realloc(p, SIZE_MAX)
>   ERROR: malloc: failed 3 out of 12 tests
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  test/self/Kconfig  |  5 +++
>  test/self/Makefile |  1 +
>  test/self/malloc.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+)
>  create mode 100644 test/self/malloc.c

Applied, thanks

Sascha

> 
> diff --git a/test/self/Kconfig b/test/self/Kconfig
> index 3340a9146ee2..cf11efe54486 100644
> --- a/test/self/Kconfig
> +++ b/test/self/Kconfig
> @@ -32,6 +32,11 @@ config SELFTEST_ENABLE_ALL
>  	help
>  	  Selects all self-tests compatible with current configuration
>  
> +config SELFTEST_MALLOC
> +	bool "malloc() selftest"
> +	help
> +	  Tests barebox memory allocator
> +
>  config SELFTEST_PRINTF
>  	bool "printf selftest"
>  	help
> diff --git a/test/self/Makefile b/test/self/Makefile
> index 05a2a6a236ec..65d01596b806 100644
> --- a/test/self/Makefile
> +++ b/test/self/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  obj-$(CONFIG_SELFTEST) += core.o
> +obj-$(CONFIG_SELFTEST_MALLOC) += malloc.o
>  obj-$(CONFIG_SELFTEST_PRINTF) += printf.o
>  obj-$(CONFIG_SELFTEST_PROGRESS_NOTIFIER) += progress-notifier.o
>  obj-$(CONFIG_SELFTEST_OF_MANIPULATION) += of_manipulation.o of_manipulation.dtb.o
> diff --git a/test/self/malloc.c b/test/self/malloc.c
> new file mode 100644
> index 000000000000..c25b416b9751
> --- /dev/null
> +++ b/test/self/malloc.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <common.h>
> +#include <bselftest.h>
> +#include <malloc.h>
> +#include <memory.h>
> +#include <linux/sizes.h>
> +
> +BSELFTEST_GLOBALS();
> +
> +static void __expect(bool cond, bool expect,
> +		     const char *condstr, const char *func, int line)
> +{
> +	total_tests++;
> +	if (cond != expect) {
> +		failed_tests++;
> +		printf("%s:%d: %s to %s\n", func, line,
> +		       expect ? "failed" : "unexpectedly succeeded",
> +		       condstr);
> +	}
> +}
> +
> +#define expect_alloc_ok(cond) \
> +	__expect((cond), true, #cond, __func__, __LINE__)
> +
> +#define expect_alloc_fail(cond) \
> +	__expect((cond), false, #cond, __func__, __LINE__)
> +
> +static void test_malloc(void)
> +{
> +	size_t mem_malloc_size = mem_malloc_end() - mem_malloc_start();
> +	u8 *p, *tmp;
> +
> +	pr_debug("mem_malloc_size = %zu\n", mem_malloc_size);
> +
> +	/* System libc when built for sandbox may have overcommit, so
> +	 * doing very big allocations without actual use may succeed
> +	 * unlike in-barebox allocators, so skip these tests in that
> +	 * case
> +	 */
> +	if (IS_ENABLED(CONFIG_MALLOC_LIBC)) {
> +		pr_info("built with host libc allocator: Skipping tests that may trigger overcommit\n");
> +		mem_malloc_size = 0;
> +	}
> +
> +	expect_alloc_ok(p = malloc(1));
> +	free(p);
> +
> +	if (mem_malloc_size) {
> +		expect_alloc_fail(malloc(SIZE_MAX));
> +
> +		if (0xf0000000 > mem_malloc_size) {
> +			expect_alloc_fail((tmp = malloc(0xf0000000)));
> +			free(tmp);
> +		}
> +	} else {
> +		skipped_tests += 2;
> +	}
> +
> +	p = realloc(NULL, 1);
> +	expect_alloc_ok(p = realloc(NULL, 1));
> +
> +	*p = 0x42;
> +
> +	expect_alloc_ok(tmp = realloc(p, 2));
> +
> +	p = tmp;
> +	__expect(*p == 0x42, true, "reread after realloc", __func__, __LINE__);
> +
> +	if (mem_malloc_size) {
> +		expect_alloc_fail(tmp = realloc(p, mem_malloc_size));
> +
> +		if (0xf0000000 > mem_malloc_size)
> +			expect_alloc_fail((tmp = realloc(p, 0xf0000000)));
> +
> +		expect_alloc_fail(tmp = realloc(p, SIZE_MAX));
> +
> +	} else {
> +		skipped_tests += 3;
> +	}
> +
> +	free(p);
> +
> +	expect_alloc_ok(p = malloc(0));
> +	expect_alloc_ok(tmp = malloc(0));
> +
> +	__expect(p != tmp, true, "allocate distinct 0-size buffers", __func__, __LINE__);
> +}
> +bselftest(core, test_malloc);
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-05-24  7:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-23  6:27 [PATCH master 1/2] test: self: add basic testing for malloc() Ahmad Fatoum
2022-05-23  6:27 ` [PATCH master 2/2] tlsf: fix internal overflow trying to allocate big buffers Ahmad Fatoum
2022-05-24  7:04 ` [PATCH master 1/2] test: self: add basic testing for malloc() Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox