Re: [PATCH] ram: stm32mp1: bound the invalid-argument message in stm32mp1_ddr_subcmd()

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