[PATCH] cmd: binop: validate inputs and fix heap overflow

[PATCH] cmd: binop: validate inputs and fix heap overflow

[Maxin B. John]

The binop command never checked the parsed count, hex strings or the
return value of hextoul(). A hex string longer than the count would
overrun the heap buffer and corrupt memory. Bad input was accepted
silently and the command still returned success.

Fix the count check. Reject hex strings that are malformed or too long.
Check every allocation and return a failure code when something goes
wrong.

Signed-off-by: Maxin B. John <maxin.john@gmail.com>
---
 cmd/binop.c | 87 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 30 deletions(-)

diff --git a/cmd/binop.c b/cmd/binop.c
index 10d91b5dbf2e..ba109fc6b4f3 100644
--- a/cmd/binop.c
+++ b/cmd/binop.c
@@ -2,6 +2,7 @@
 
 #include <command.h>
 #include <env.h>
+#include <errno.h>
 #include <hexdump.h>
 #include <malloc.h>
 #include <mapmem.h>
@@ -14,13 +15,15 @@ enum {
 	OP_ID_OR,
 };
 
-void write_to_env_var(char *varname, u8 *result, ulong len)
+int write_to_env_var(char *varname, u8 *result, ulong len)
 {
 	char *str_output;
 	char *str_ptr;
-	int i;
+	int i, ret;
 
 	str_output = malloc(len * 2 + 1);
+	if (!str_output)
+		return -ENOMEM;
 	str_ptr = str_output;
 
 	for (i = 0; i < len; i++) {
@@ -28,40 +31,70 @@ void write_to_env_var(char *varname, u8 *result, ulong len)
 		str_ptr += 2;
 	}
 	*str_ptr = '\0';
-	env_set(varname, str_output);
-
+	ret = env_set(varname, str_output);
 	free(str_output);
+
+	return ret;
 }
 
-void read_from_env_var(char *varname, u8 *result)
+int read_from_env_var(char *varname, u8 *result, ulong len)
 {
 	char *str_value;
+	size_t slen;
 
 	str_value = env_get(varname);
-	if (str_value)
-		hex2bin(result, str_value, strlen(str_value) / 2);
-	else
-		hex2bin(result, varname, strlen(varname) / 2);
+	if (!str_value)
+		str_value = varname;
+
+	/* Shorter hex strings leave the (zeroed) tail of result untouched */
+	slen = strlen(str_value);
+	if ((slen & 1) || slen / 2 > len || hex2bin(result, str_value, slen / 2)) {
+		printf("Error: '%s' is not an env var or hex string of up to %lu bytes\n",
+		       varname, len);
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
-void read_from_mem(ulong addr, u8 *result, ulong len)
+int read_from_mem(char *varname, u8 *result, ulong len)
 {
+	ulong addr;
 	u8 *src;
 
+	if (strict_strtoul(varname, 16, &addr)) {
+		printf("Error: invalid address '%s'\n", varname);
+		return -EINVAL;
+	}
 	src = map_sysmem(addr, len);
 	memcpy(result, src, len);
 	unmap_sysmem(src);
+
+	return 0;
 }
 
-void write_to_mem(char *varname, u8 *result, ulong len)
+int write_to_mem(char *varname, u8 *result, ulong len)
 {
 	ulong addr;
 	u8 *buf;
 
-	addr = hextoul(varname, NULL);
+	if (strict_strtoul(varname, 16, &addr)) {
+		printf("Error: invalid address '%s'\n", varname);
+		return -EINVAL;
+	}
 	buf = map_sysmem(addr, len);
 	memcpy(buf, result, len);
 	unmap_sysmem(buf);
+
+	return 0;
+}
+
+static int read_src(char *arg, u8 *result, ulong len)
+{
+	if (*arg == '*')
+		return read_from_mem(arg + 1, result, len); /* Skip asterisk */
+
+	return read_from_env_var(arg, result, len);
 }
 
 static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -70,7 +103,7 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
 	ulong len;
 	u8 *result, *src1, *src2;
 	char *oparg, *lenarg, *src1arg, *src2arg, *destarg;
-	int i, op;
+	int i, op, ret = CMD_RET_FAILURE;
 
 	if (argc < 5)
 		return CMD_RET_USAGE;
@@ -89,22 +122,15 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
 	else
 		return CMD_RET_USAGE;
 
-	len = dectoul(lenarg, NULL);
-
-	src1 = malloc(len);
-	src2 = malloc(len);
-
-	if (*src1arg == '*')
-		read_from_mem(hextoul(src1arg + 1, NULL), src1, len);
-	else
-		read_from_env_var(src1arg, src1);
-
-	if (*src2arg == '*')
-		read_from_mem(hextoul(src2arg + 1, NULL), src2, len);
-	else
-		read_from_env_var(src2arg, src2);
+	if (strict_strtoul(lenarg, 10, &len) || !len)
+		return CMD_RET_USAGE;
 
+	src1 = calloc(1, len);
+	src2 = calloc(1, len);
 	result = malloc(len);
+	if (!src1 || !src2 || !result ||
+	    read_src(src1arg, src1, len) || read_src(src2arg, src2, len))
+		goto exit;
 
 	switch (op) {
 	case OP_ID_XOR:
@@ -129,21 +155,22 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
 		}
 		puts("\n");
 
+		ret = CMD_RET_SUCCESS;
 		goto exit;
 	}
 
 	destarg = argv[5];
 
 	if (*destarg == '*')
-		write_to_mem(destarg + 1, result, len); /* Skip asterisk */
+		ret = write_to_mem(destarg + 1, result, len); /* Skip asterisk */
 	else
-		write_to_env_var(destarg, result, len);
+		ret = write_to_env_var(destarg, result, len);
 exit:
 	free(result);
 	free(src2);
 	free(src1);
 
-	return 0;
+	return ret ? CMD_RET_FAILURE : CMD_RET_SUCCESS;
 }
 
 U_BOOT_CMD(
-- 
2.47.3

Re: [PATCH] cmd: binop: validate inputs and fix heap overflow

[Simon Glass]

Hi Maxin,

On 2026-10-27T15:12:29, Maxin B. John <maxin.john@gmail.com> wrote:
> cmd: binop: validate inputs and fix heap overflow
>
> The binop command never checked the parsed count, hex strings or the
> return value of hextoul(). A hex string longer than the count would
> overrun the heap buffer and corrupt memory. Bad input was accepted
> silently and the command still returned success.
>
> Fix the count check. Reject hex strings that are malformed or too long.

The existing code has no count check, so this adds one. Please can you
say what is now accepted, e.g. 'Require the count to be a non-zero
number'.

> Check every allocation and return a failure code when something goes
> wrong.
>
> Signed-off-by: Maxin B. John <maxin.john@gmail.com>
>
>  cmd/binop.c | 87 ++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 57 insertions(+), 30 deletions(-)

> diff --git a/cmd/binop.c b/cmd/binop.c
> @@ -14,13 +15,15 @@ enum {
> -void write_to_env_var(char *varname, u8 *result, ulong len)
> +int write_to_env_var(char *varname, u8 *result, ulong len)

These four helpers are only used in this file and have no prototypes.
Since you are changing their signatures anyway, please can you make
them static, as you did with read_src()?

> diff --git a/cmd/binop.c b/cmd/binop.c
> @@ -28,40 +31,70 @@ void write_to_env_var(char *varname, u8 *result, ulong len)
> +	if ((slen & 1) || slen / 2 > len || hex2bin(result, str_value, slen / 2)) {
> +		printf("Error: '%s' is not an env var or hex string of up to %lu bytes\n",
> +		       varname, len);

If 'varname' is an env var whose value is too long or not valid hex,
this message says it is not an env var, which is confusing. It would
be better to print a different message in that case, or at least show
the value that failed to parse.

> diff --git a/cmd/binop.c b/cmd/binop.c
> @@ -89,22 +122,15 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
> +	if (strict_strtoul(lenarg, 10, &len) || !len)
> +		return CMD_RET_USAGE;

There are no tests for binop at present. A test in test/cmd/ covering
the overflow case and the new error paths would be good, perhaps as a
follow-up.

Regards,
Simon

[PATCH v2] cmd: binop: validate inputs and fix heap overrun

[Maxin B. John]

The binop command never checked the parsed count, hex strings or the
return value of hextoul(). A hex string longer than the count would
overrun the heap buffer and corrupt memory. Bad input was accepted
silently and the command still returned success.

Require the count to be a non-zero decimal number. Reject hex strings
that have an odd number of digits, contain non-hex characters or are
longer than count bytes, reporting the offending env var value where
there is one. Check every allocation. Return a failure code when
something goes wrong.

Make the file-local helpers static while changing their signatures.

Signed-off-by: Maxin B. John <maxin.john@gmail.com>
---
Changes in v2:
  - Explain what count is now accepted in the commit message
  - Make the env/mem helpers static
  - Print a separate error, showing the value, when an env var holds an
    invalid or too-long hex string
---
 cmd/binop.c | 93 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 30 deletions(-)

diff --git a/cmd/binop.c b/cmd/binop.c
index 10d91b5dbf2e..ee1ad671f3c6 100644
--- a/cmd/binop.c
+++ b/cmd/binop.c
@@ -2,6 +2,7 @@
 
 #include <command.h>
 #include <env.h>
+#include <errno.h>
 #include <hexdump.h>
 #include <malloc.h>
 #include <mapmem.h>
@@ -14,13 +15,15 @@ enum {
 	OP_ID_OR,
 };
 
-void write_to_env_var(char *varname, u8 *result, ulong len)
+static int write_to_env_var(char *varname, u8 *result, ulong len)
 {
 	char *str_output;
 	char *str_ptr;
-	int i;
+	int i, ret;
 
 	str_output = malloc(len * 2 + 1);
+	if (!str_output)
+		return -ENOMEM;
 	str_ptr = str_output;
 
 	for (i = 0; i < len; i++) {
@@ -28,40 +31,76 @@ void write_to_env_var(char *varname, u8 *result, ulong len)
 		str_ptr += 2;
 	}
 	*str_ptr = '\0';
-	env_set(varname, str_output);
-
+	ret = env_set(varname, str_output);
 	free(str_output);
+
+	return ret;
 }
 
-void read_from_env_var(char *varname, u8 *result)
+static int read_from_env_var(char *varname, u8 *result, ulong len)
 {
 	char *str_value;
+	size_t slen;
+	bool is_env;
 
 	str_value = env_get(varname);
-	if (str_value)
-		hex2bin(result, str_value, strlen(str_value) / 2);
-	else
-		hex2bin(result, varname, strlen(varname) / 2);
+	is_env = !!str_value;
+	if (!is_env)
+		str_value = varname;
+
+	/* Shorter hex strings leave the (zeroed) tail of result untouched */
+	slen = strlen(str_value);
+	if ((slen & 1) || slen / 2 > len || hex2bin(result, str_value, slen / 2)) {
+		if (is_env)
+			printf("Error: env var '%s' value '%s' is not a hex string of up to %lu bytes\n",
+			       varname, str_value, len);
+		else
+			printf("Error: '%s' is not an env var or hex string of up to %lu bytes\n",
+			       varname, len);
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
-void read_from_mem(ulong addr, u8 *result, ulong len)
+static int read_from_mem(char *varname, u8 *result, ulong len)
 {
+	ulong addr;
 	u8 *src;
 
+	if (strict_strtoul(varname, 16, &addr)) {
+		printf("Error: invalid address '%s'\n", varname);
+		return -EINVAL;
+	}
 	src = map_sysmem(addr, len);
 	memcpy(result, src, len);
 	unmap_sysmem(src);
+
+	return 0;
 }
 
-void write_to_mem(char *varname, u8 *result, ulong len)
+static int write_to_mem(char *varname, u8 *result, ulong len)
 {
 	ulong addr;
 	u8 *buf;
 
-	addr = hextoul(varname, NULL);
+	if (strict_strtoul(varname, 16, &addr)) {
+		printf("Error: invalid address '%s'\n", varname);
+		return -EINVAL;
+	}
 	buf = map_sysmem(addr, len);
 	memcpy(buf, result, len);
 	unmap_sysmem(buf);
+
+	return 0;
+}
+
+static int read_src(char *arg, u8 *result, ulong len)
+{
+	if (*arg == '*')
+		return read_from_mem(arg + 1, result, len); /* Skip asterisk */
+
+	return read_from_env_var(arg, result, len);
 }
 
 static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -70,7 +109,7 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
 	ulong len;
 	u8 *result, *src1, *src2;
 	char *oparg, *lenarg, *src1arg, *src2arg, *destarg;
-	int i, op;
+	int i, op, ret = CMD_RET_FAILURE;
 
 	if (argc < 5)
 		return CMD_RET_USAGE;
@@ -89,22 +128,15 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
 	else
 		return CMD_RET_USAGE;
 
-	len = dectoul(lenarg, NULL);
-
-	src1 = malloc(len);
-	src2 = malloc(len);
-
-	if (*src1arg == '*')
-		read_from_mem(hextoul(src1arg + 1, NULL), src1, len);
-	else
-		read_from_env_var(src1arg, src1);
-
-	if (*src2arg == '*')
-		read_from_mem(hextoul(src2arg + 1, NULL), src2, len);
-	else
-		read_from_env_var(src2arg, src2);
+	if (strict_strtoul(lenarg, 10, &len) || !len)
+		return CMD_RET_USAGE;
 
+	src1 = calloc(1, len);
+	src2 = calloc(1, len);
 	result = malloc(len);
+	if (!src1 || !src2 || !result ||
+	    read_src(src1arg, src1, len) || read_src(src2arg, src2, len))
+		goto exit;
 
 	switch (op) {
 	case OP_ID_XOR:
@@ -129,21 +161,22 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
 		}
 		puts("\n");
 
+		ret = CMD_RET_SUCCESS;
 		goto exit;
 	}
 
 	destarg = argv[5];
 
 	if (*destarg == '*')
-		write_to_mem(destarg + 1, result, len); /* Skip asterisk */
+		ret = write_to_mem(destarg + 1, result, len); /* Skip asterisk */
 	else
-		write_to_env_var(destarg, result, len);
+		ret = write_to_env_var(destarg, result, len);
 exit:
 	free(result);
 	free(src2);
 	free(src1);
 
-	return 0;
+	return ret ? CMD_RET_FAILURE : CMD_RET_SUCCESS;
 }
 
 U_BOOT_CMD(
-- 
2.47.3

Re: [PATCH] cmd: binop: validate inputs and fix heap overflow

[Maxin John]

Hi Simon,

Thanks for the review.

On Wed, Oct 28, 2026 at 12:49:53AM -0700, Simon Glass wrote:
> Hi Maxin,
> 
> On 2026-10-27T15:12:29, Maxin B. John <maxin.john@gmail.com> wrote:
> > cmd: binop: validate inputs and fix heap overflow
> >
> > The binop command never checked the parsed count, hex strings or the
> > return value of hextoul(). A hex string longer than the count would
> > overrun the heap buffer and corrupt memory. Bad input was accepted
> > silently and the command still returned success.
> >
> > Fix the count check. Reject hex strings that are malformed or too long.
> 
> The existing code has no count check, so this adds one. Please can you
> say what is now accepted, e.g. 'Require the count to be a non-zero
> number'.

Agree. v2 says: "Require the count to be a non-zero decimal number.
Reject hex strings that have an odd number of digits, contain non-hex
characters or are longer than count bytes."
 
> > Check every allocation and return a failure code when something goes
> > wrong.
> >
> > Signed-off-by: Maxin B. John <maxin.john@gmail.com>
> >
> >  cmd/binop.c | 87 ++++++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 57 insertions(+), 30 deletions(-)
> 
> > diff --git a/cmd/binop.c b/cmd/binop.c
> > @@ -14,13 +15,15 @@ enum {
> > -void write_to_env_var(char *varname, u8 *result, ulong len)
> > +int write_to_env_var(char *varname, u8 *result, ulong len)
> 
> These four helpers are only used in this file and have no prototypes.
> Since you are changing their signatures anyway, please can you make
> them static, as you did with read_src()?

Updated in v2.
 
> > diff --git a/cmd/binop.c b/cmd/binop.c
> > @@ -28,40 +31,70 @@ void write_to_env_var(char *varname, u8 *result, ulong len)
> > +	if ((slen & 1) || slen / 2 > len || hex2bin(result, str_value, slen / 2)) {
> > +		printf("Error: '%s' is not an env var or hex string of up to %lu bytes\n",
> > +		       varname, len);
> 
> If 'varname' is an env var whose value is too long or not valid hex,
> this message says it is not an env var, which is confusing. It would
> be better to print a different message in that case, or at least show
> the value that failed to parse.

Good catch. v2 prints a separate message for that case, including the
value. Kept the existing message for arguments that are neither env
var nor valid hex. 

> > diff --git a/cmd/binop.c b/cmd/binop.c
> > @@ -89,22 +122,15 @@ static int do_binop(struct cmd_tbl *cmdtp, int flag, int argc,
> > +	if (strict_strtoul(lenarg, 10, &len) || !len)
> > +		return CMD_RET_USAGE;
> 
> There are no tests for binop at present. A test in test/cmd/ covering
> the overflow case and the new error paths would be good, perhaps as a
> follow-up.

Yes, I will send that as a follow-up.
 
> Regards,
> Simon

Thanks & Regards,
Maxin