[PATCH v1 1/2] lib: hashtable: fix integer overflow in himport_r

[PATCH v1 1/2] lib: hashtable: fix integer overflow in himport_r

[Aristo Chen]

When size == SIZE_MAX, the expression malloc(size + 1) wraps to
malloc(0) due to unsigned integer overflow. malloc(0) may return a
non-NULL pointer, causing the subsequent memcpy(data, env, size) to
write SIZE_MAX bytes into a zero-byte allocation.

This is reachable from the U-Boot console via "env import", where size
is taken directly from a user-supplied hex argument.

Add an explicit check for SIZE_MAX before the malloc call and return
EINVAL.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 lib/hashtable.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/hashtable.c b/lib/hashtable.c
index 75c263b5053..902fa6f3e98 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -820,6 +820,13 @@ int himport_r(struct hsearch_data *htab,
 		return 0;
 	}
 
+	/* Check for potential integer overflow */
+	if (size == SIZE_MAX) {
+		debug("%s: size too large, would overflow\n", __func__);
+		__set_errno(EINVAL);
+		return 0;
+	}
+
 	/* we allocate new space to make sure we can write to the array */
 	if ((data = malloc(size + 1)) == NULL) {
 		debug("himport_r: can't malloc %lu bytes\n", (ulong)size + 1);
-- 
2.43.0

[PATCH v1 2/2] test: env: add test for himport_r SIZE_MAX overflow guard

[Aristo Chen]

Add a unit test that verifies himport_r rejects SIZE_MAX as the size
argument, ensuring the integer overflow guard added to lib/hashtable.c
is exercised and not accidentally regressed.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 test/env/hashtable.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/env/hashtable.c b/test/env/hashtable.c
index 16e49358888..ce9689f46d7 100644
--- a/test/env/hashtable.c
+++ b/test/env/hashtable.c
@@ -122,3 +122,20 @@ static int env_test_htab_deletes(struct unit_test_state *uts)
 	return 0;
 }
 ENV_TEST(env_test_htab_deletes, 0);
+
+/* Verify himport_r rejects SIZE_MAX to prevent integer overflow in malloc */
+static int env_test_htab_import_overflow(struct unit_test_state *uts)
+{
+	struct hsearch_data htab;
+
+	memset(&htab, 0, sizeof(htab));
+	ut_asserteq(1, hcreate_r(SIZE, &htab));
+
+	/* SIZE_MAX would cause malloc(size + 1) to wrap to malloc(0) */
+	ut_asserteq(0, himport_r(&htab, "", SIZE_MAX, '\0', 0, 0, 0, NULL));
+
+	hdestroy_r(&htab);
+	return 0;
+}
+
+ENV_TEST(env_test_htab_import_overflow, 0);
-- 
2.43.0

Re: [PATCH v1 1/2] lib: hashtable: fix integer overflow in himport_r

[Stefan Monnier]

> +	/* Check for potential integer overflow */
> +	if (size == SIZE_MAX) {
> +		debug("%s: size too large, would overflow\n", __func__);
> +		__set_errno(EINVAL);
> +		return 0;
> +	}
> +
>  	/* we allocate new space to make sure we can write to the array */
>  	if ((data = malloc(size + 1)) == NULL) {
>  		debug("himport_r: can't malloc %lu bytes\n", (ulong)size + 1);

Rather than depend on SIZE_MAX being the right boundary, can we do
a check along the lines of `size + 1 > size`?


=== Stefan

Re: [PATCH v1 1/2] lib: hashtable: fix integer overflow in himport_r

[Rasmus Villemoes]

On Wed, Apr 08 2026, Aristo Chen <aristo.chen@canonical.com> wrote:

> When size == SIZE_MAX, the expression malloc(size + 1) wraps to
> malloc(0) due to unsigned integer overflow. malloc(0) may return a
> non-NULL pointer, causing the subsequent memcpy(data, env, size) to
> write SIZE_MAX bytes into a zero-byte allocation.
>
> This is reachable from the U-Boot console via "env import", where size
> is taken directly from a user-supplied hex argument.
>
> Add an explicit check for SIZE_MAX before the malloc call and return
> EINVAL.
>
> Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
> ---
>  lib/hashtable.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index 75c263b5053..902fa6f3e98 100644
> --- a/lib/hashtable.c
> +++ b/lib/hashtable.c
> @@ -820,6 +820,13 @@ int himport_r(struct hsearch_data *htab,
>  		return 0;
>  	}
>  
> +	/* Check for potential integer overflow */
> +	if (size == SIZE_MAX) {
> +		debug("%s: size too large, would overflow\n", __func__);
> +		__set_errno(EINVAL);
> +		return 0;
> +	}
> +

Well, you can corrupt arbitrary memory from the u-boot shell, so "taken
directly from a user-supplied hex argument" is not really a very
compelling argument in the context of U-Boot.

Instead of adding such ad hoc checks that mostly just increase code size
a little, I think it's better to zoom out and see what this really
does. And this is ripe for adding a memdup_nul() helper (linux has that
under the name kmemdup_nul). If we add that, we can do the overflow
check inside that in that one place, and we can convert a lot of similar
users all over the tree, and eliminate quite a lot of #loc.

I'll try to write something.

Rasmus