linux-kernel - Re: [PATCH] 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] [thread-next>] [day] [month] [year] [list]

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

On 06/03/2024 21.42, Andrew Morton wrote:
> On Wed,  6 Mar 2024 13:24:52 +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.
>>
>> ...
>>
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -319,12 +319,20 @@ static char xbc_namebuf[XBC_KEYLEN_MAX] __initdata;
>>  
>>  #define rest(dst, end) ((end) > (dst) ? (end) - (dst) : 0)
>>  
>> +static int has_space(const char *v)
>> +{
>> +	for (; *v; v++)
>> +		if (isspace(*v))
>> +			return 1;
>> +	return 0;
>> +}
> 
> Do we already have something which does this?

Well, 'value[strcspn(value, " \t\r\n")] ? "\"" : ""' would be a
oneliner, but not particularly readable. Also that list of characters
doesn't necessarily match isspace(), see below.

> Could do strchr(' ')||strchr('\t')
> 
> Do we really support tab separation here?  I doubt if that gets used or
> tested much.

Indeed, I did consider just doing strchr(' '), but ended up with
something based on isspace() as that is what next_arg() in lib/cmdline.c
uses, and also what lib/bootconfig.c allows through (together with
isprint() of course). But yes, \t, \r and \n are unlikely to be used,
even more so \f and \v, but perhaps somebody has 0xa0 by accident (nbsp
in latin1) which our isspace() also allows.

I didn't really want to risk a "prettify the 99% normal use cases" patch
breaking some odd existing setup, so went with the full isspace() check
which really wanted to be done in a helper.

> This function could be __init.

True, but the compiler inlines it to its only caller.

Rasmus