[PATCH] board: nanopi2: fix bd_update_env() cmdline buffer overflow

[Stefan Bosch]

Hello Ngo,

thanks a lot for the detailed report and the patch. I will have a closer 
look at it the next days.

Regards
Stefan


On 28.03.26 06:15, Ngo Luong Thanh Tra wrote:
> Replace unbounded strcpy()/sprintf() calls with snprintf() and
> check the return value against remaining buffer capacity at each
> append step. The previous size guard did not account for
> subsequent dpi suffix, remaining bootargs tail, and bootdev
> token appends, allowing overflow when those later writes exceed
> the remaining space.
> 
> Fixes: d1611086e005 ("arm: add support for SoC s5p4418 (cpu) / nanopi2 board")
> To: u-boot at lists.denx.de
> 
> Signed-off-by: Ngo Luong Thanh Tra <S4210155 at student.rmit.edu.au>
> ---
> 
>   board/friendlyarm/nanopi2/board.c | 49 ++++++++++++++++++++-----------
>   1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/board/friendlyarm/nanopi2/board.c b/board/friendlyarm/nanopi2/board.c
> index 4dff32e10d6..eb10cd5143d 100644
> --- a/board/friendlyarm/nanopi2/board.c
> +++ b/board/friendlyarm/nanopi2/board.c
> @@ -328,6 +328,7 @@ static void bd_update_env(void)
>   #define CMDLINE_LCD		" lcd="
>   	char cmdline[CONFIG_SYS_CBSIZE];
>   	int n = 1;
> +	int ret;
>   
>   	if (rootdev != CONFIG_ROOT_DEV && !env_get("firstboot")) {
>   		env_set_ulong("rootdev", rootdev);
> @@ -347,49 +348,63 @@ static void bd_update_env(void)
>   	else
>   		cmdline[0] = '\0';
>   
> -	if ((n + strlen(name) + sizeof(CMDLINE_LCD)) > sizeof(cmdline)) {
> -		printf("Error: `bootargs' is too large (%d)\n", n);
> -		goto __exit;
> -	}
> -
>   	if (bootargs) {
>   		p = strstr(bootargs, CMDLINE_LCD);
>   		if (p) {
>   			n = (p - bootargs);
>   			p += strlen(CMDLINE_LCD);
>   		}
> -		strncpy(cmdline, bootargs, n);
> +		ret = snprintf(cmdline, sizeof(cmdline), "%.*s", n, bootargs);
> +		if (ret < 0 || ret >= (int)sizeof(cmdline))
> +			goto __exit;
>   	}
>   
>   	/* add `lcd=NAME,NUMdpi' */
> -	strncpy(cmdline + n, CMDLINE_LCD, strlen(CMDLINE_LCD));
> -	n += strlen(CMDLINE_LCD);
> +	ret = snprintf(cmdline + n, sizeof(cmdline) - n, "%s", CMDLINE_LCD);
> +	if (ret < 0 || ret >= (int)(sizeof(cmdline) - n))
> +		goto __exit;
> +	n += ret;
>   
> -	strcpy(cmdline + n, name);
> -	n += strlen(name);
> +	ret = snprintf(cmdline + n, sizeof(cmdline) - n, "%s", name);
> +	if (ret < 0 || ret >= (int)(sizeof(cmdline) - n))
> +		goto __exit;
> +	n += ret;
>   
>   	if (lcddpi) {
> -		n += sprintf(cmdline + n, ",%sdpi", lcddpi);
> +		ret = snprintf(cmdline + n, sizeof(cmdline) - n, ",%sdpi", lcddpi);
> +		if (ret < 0 || ret >= (int)(sizeof(cmdline) - n))
> +			goto __exit;
> +		n += ret;
>   	} else {
>   		int dpi = bd_get_lcd_density();
>   
> -		if (dpi > 0 && dpi < 600)
> -			n += sprintf(cmdline + n, ",%ddpi", dpi);
> +		if (dpi > 0 && dpi < 600) {
> +			ret = snprintf(cmdline + n, sizeof(cmdline) - n, ",%ddpi", dpi);
> +			if (ret < 0 || ret >= (int)(sizeof(cmdline) - n))
> +				goto __exit;
> +			n += ret;
> +		}
>   	}
>   
>   	/* copy remaining of bootargs */
>   	if (p) {
>   		p = strstr(p, " ");
>   		if (p) {
> -			strcpy(cmdline + n, p);
> -			n += strlen(p);
> +			ret = snprintf(cmdline + n, sizeof(cmdline) - n, "%s", p);
> +			if (ret < 0 || ret >= (int)(sizeof(cmdline) - n))
> +				goto __exit;
> +			n += ret;
>   		}
>   	}
>   
>   	/* append `bootdev=2' */
>   #define CMDLINE_BDEV	" bootdev="
> -	if (rootdev > 0 && !strstr(cmdline, CMDLINE_BDEV))
> -		n += sprintf(cmdline + n, "%s2", CMDLINE_BDEV);
> +	if (rootdev > 0 && !strstr(cmdline, CMDLINE_BDEV)) {
> +		ret = snprintf(cmdline + n, sizeof(cmdline) - n, "%s2", CMDLINE_BDEV);
> +		if (ret < 0 || ret >= (int)(sizeof(cmdline) - n))
> +			goto __exit;
> +		n += ret;
> +	}
>   
>   	/* finally, let's update uboot env & save it */
>   	if (bootargs && strncmp(cmdline, bootargs, sizeof(cmdline))) {