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 <[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'. 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 <[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()? 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
