On 8/10/26 10:08, ngotra2710 wrote:
> From: Ngo Luong Thanh Tra <[email protected]>
>
> stm32mp1_ddr_subcmd() formats a rejected argument into a 50-byte stack
> buffer with an unbounded sprintf():
>
> char string[50] = "";
> ...
> sprintf(string, "invalid argument %s", argv[1]);
>
> argv[1] comes from cli_simple_parse_line() over a console line held in
> buffer[CONFIG_SYS_CBSIZE], which is commonly 256 bytes or more. The
> fixed prefix takes 17 bytes, leaving 32 for the argument and its NUL, so
> any invalid argument longer than 32 characters writes past the end of
> string and corrupts the stack of the DDR interactive console.
>
> Use snprintf() with sizeof(string). The sibling call a few lines below
> formats only integers and cannot overflow, so it is left alone.
>
> Fixes: 0d447524425e ("stm32mp1: ram: add tests in DDR interactive mode")
> Signed-off-by: Ngo Luong Thanh Tra <[email protected]>
> Cc: Patrick Delaunay <[email protected]>
> Cc: Patrice Chotard <[email protected]>
> Cc: Tom Rini <[email protected]>
> ---
> Note, not for the commit message: the same 50-byte buffer is handed to the
> test functions in drivers/ram/stm32mp1/stm32mp1_tests.c, which write into it
> with further unbounded sprintf() calls, several of them formatting %s from
> argv (lines 28, 33, 38, 60, 81, 86, 90 and 113). Those cannot be fixed the
> same way because the buffer arrives there as a char * parameter, so sizeof()
> is not available at the call site. Fixing them properly needs either a size
> parameter alongside the pointer or a shared constant for the buffer length,
> which seemed like a separate change rather than something to fold in here.
> Happy to follow up with that if you would like it done in one go.
Hi Ngo
Feel free to submit another fix regarding the issue found in stm32mp1_tests.c
>
> drivers/ram/stm32mp1/stm32mp1_interactive.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ram/stm32mp1/stm32mp1_interactive.c
> b/drivers/ram/stm32mp1/stm32mp1_interactive.c
> index 6340afbb87..3154fde6bf 100644
> --- a/drivers/ram/stm32mp1/stm32mp1_interactive.c
> +++ b/drivers/ram/stm32mp1/stm32mp1_interactive.c
> @@ -334,8 +334,8 @@ static void stm32mp1_ddr_subcmd(struct ddr_info *priv,
>
> if ((strict_strtoul(argv[1], 0, &value) < 0) ||
> value >= array_nb) {
> - sprintf(string, "invalid argument %s",
> - argv[1]);
> + snprintf(string, sizeof(string), "invalid argument %s",
> + argv[1]);
> result = TEST_FAILED;
> goto end;
> }
>
> base-commit: ece349ade2973e220f524ce59e59711cc919263f
> prerequisite-patch-id: 53e6ea149bf19676d9f2becfa13157b07bee89fc
Reviewed-by: Patrice Chotard <[email protected]>
Thanks
Patrice
