Re: [PATCH 3/3] common: cli_hush: fix console_buffer overflow on boot retry

[Rasmus Villemoes <ravi@prevas.dk>]

On Sat, Mar 28 2026, Ngo Luong Thanh Tra <ngotra27101996@gmail.com> wrote:

> Replace strcpy() with strlcpy() when injecting the boot retry
> command into console_buffer. Add a BUILD_BUG_ON() to catch at
> compile time any configuration where CONFIG_SYS_CBSIZE is smaller
> than the retry command string, and use a named constant for the
> command so that the size check stays in sync if the string is
> ever changed.
>
> Fixes: 657e19f8f2dd ("cli_hush: support running bootcmd on boot retry")
> Signed-off-by: Ngo Luong Thanh Tra <S4210155@student.rmit.edu.au>
> To: u-boot@lists.denx.de
> ---
>
>  common/cli_hush.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/common/cli_hush.c b/common/cli_hush.c
> index 7bd6943d3ed..6141c2959df 100644
> --- a/common/cli_hush.c
> +++ b/common/cli_hush.c
> @@ -84,6 +84,7 @@
>  #include <cli_hush.h>
>  #include <command.h>        /* find_cmd */
>  #include <asm/global_data.h>
> +#include <linux/build_bug.h>
>  #endif
>  #ifndef __U_BOOT__
>  #include <ctype.h>     /* isalpha, isdigit */
> @@ -1029,7 +1030,10 @@ static void get_user_input(struct in_str *i)
>  #  ifdef CONFIG_RESET_TO_RETRY
>  	  do_reset(NULL, 0, 0, NULL);
>  #  elif IS_ENABLED(CONFIG_RETRY_BOOTCMD)
> -	strcpy(console_buffer, "run bootcmd\n");
> +	static const char retry_cmd[] = "run bootcmd\n";
> +
> +	BUILD_BUG_ON(sizeof(retry_cmd) - 1 > CONFIG_SYS_CBSIZE);
> +	strlcpy(console_buffer, retry_cmd, sizeof(console_buffer));

Have you compiled this?

The declaration of console_buffer in include/console.h does not include
the size, so you should get a build error like

  error: invalid application of 'sizeof' to incomplete type 'char[]'

And exactly because that declaration doesn't include the size, the -1
and the comparison to CONFIG_SYS_CBSIZE looks rather fishy.

If anything, one should start by making the size of console_buffer part
of the declaration, so that users such as here could actually do
sizeof(console_buffer), and then one should not use or need to know that
the size if defined in terms of (but not exactly equal to)
CONFIG_SYS_CBSIZE.

Also, I generally think that this whole "must use strlcpy because
safer!" is broken when everything in sight are compile-time
constants. Because the compiler knows about strcpy(), so it can optimize
a strcpy() with a literal as source into a sequence of a few immediate
stores, which is often smaller code than emitting the string literal to
.rodata.str and emitting an actual strcpy() call with all the register
save/restore that requires. It knows nothing about strlcpy().

Unfortunately, U-Boot builds with -Wno-array-bounds, so just declaring
console_buffer with its actual size is not enough to trigger a build
error with the current strcpy(). But if you want to improve stuff in
this area, do something like creating a const_strcpy() helper macro
which will enforce that

  (a) The source is a string literal
  (b) The destination is a char array of known size
  (c) Makes it a build-time error if it doesn't fit
  (d) Uses __builtin_strcpy(dst, src) to tell the compiler that this
  really is just a strcpy(), even if -fno-builtin is in effect, and let
  the compiler optimize as it sees fit - including eliminating the whole
  thing as dead stores if it sees that the destination is not actually
  used.

Something like

#define const_strcpy(d, s) ({ \
  static_assert(__same_type(d, char[]), "destination must be char array"); \
  static_assert(__same_type(s, const char[], "source must be a string literal"); \
  static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in destination"); \
  __builtin_strcpy(d, s); \
})

Rasmus