linux-kernel - Re: [PATCH v2] bootconfig: do not put quotes on cmdline items unless necessary

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sun, 10 Mar 2024 10:25:34 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] bootconfig: do not put quotes on cmdline items
 unless necessary

On Fri,  8 Mar 2024 13:44:01 +0100
Rasmus Villemoes <linux@...musvillemoes.dk> wrote:

> When trying to migrate to using bootconfig to embed the kernel's and
> PID1's command line with the kernel image itself, and so allowing
> changing that without modifying the bootloader, I noticed that
> /proc/cmdline changed from e.g.
> 
>   console=ttymxc0,115200n8 cma=128M quiet -- --log-level=notice
> 
> to
> 
>   console="ttymxc0,115200n8" cma="128M" quiet -- --log-level="notice"
> 
> The kernel parameters are parsed just fine, and the quotes are indeed
> stripped from the actual argv[] given to PID1. However, the quoting
> doesn't really serve any purpose and looks excessive, and might
> confuse some (naive) userspace tool trying to parse /proc/cmdline. So
> do not quote the value unless it contains whitespace.
> 
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> ---
> 
> v2: use strpbrk(, " \t\r\n") instead of a loop doing isspace().
> Technically not quite equivalent, but much more readable, and it's
> quite unlikely anybody has \f or \v or 0xa0 bytes in kernel command
> line arguments. Perhaps \r and \n, and maybe even \t, could also be
> dropped from that list, but those at least have some chance of
> appearing in the wild.
> 
>  init/main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index e24b0780fdff..3dd630132209 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -324,7 +324,7 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
>  {
>  	struct xbc_node *knode, *vnode;
>  	char *end = buf + size;
> -	const char *val;
> +	const char *val, *q;
>  	int ret;
>  
>  	xbc_node_for_each_key_value(root, knode, val) {
> @@ -342,8 +342,9 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
>  			continue;
>  		}
>  		xbc_array_for_each_value(vnode, val) {
> -			ret = snprintf(buf, rest(buf, end), "%s=\"%s\" ",
> -				       xbc_namebuf, val);

Can you add a comment that why strpbrk(," \t\r\n") is used here?
Such comment will help when we change how to parse the cmdline options.

Thanks,

> +			q = strpbrk(val, " \t\r\n") ? "\"" : "";
> +			ret = snprintf(buf, rest(buf, end), "%s=%s%s%s ",
> +				       xbc_namebuf, q, val, q);
>  			if (ret < 0)
>  				return ret;
>  			buf += ret;
> -- 
> 2.40.1.1.g1c60b9335d
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>