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

[Maxin John <maxin.john@gmail.com>]

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