Hi Maxin, On 2026-10-27T15:12:29, Maxin B. John <[email protected]> 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 <[email protected]> > > 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
