linux-kernel - Re: [PATCH] init: fix in-place parameter modification regression

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:	Mon, 14 Oct 2013 12:34:02 +0100
From:	Pawel Moll <pawel.moll@....com>
To:	Krzysztof Mazur <krzysiek@...lesie.net>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] init: fix in-place parameter modification regression

On Sat, 2013-10-12 at 19:05 +0100, Krzysztof Mazur wrote:
> Before commit 026cee0086fe1df4cf74691cf273062cc769617d
> ("params: <level>_initcall-like kernel parameters") the __setup
> parameter parsing code could modify parameter in the
> static_command_line buffer and such modifications were kept. After
> that commit such modifications are destroyed during per-initcall level
> parameter parsing because the same static_command_line buffer is used
> and only parameters for appropriate initcall level are parsed.
> 
> That change broke at least parsing "ubd" parameter in the ubd driver
> when the COW file is used.
> 
> Now the separate buffer is used for per-initcall parameter parsing,
> like in parsing early params.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@...lesie.net>
> ---
>
>  init/main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 63d3e8f..e5b322a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -742,12 +742,13 @@ static char *initcall_level_names[] __initdata = {
>  
>  static void __init do_initcall_level(int level)
>  {
> +	static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
>  	extern const struct kernel_param __start___param[], __stop___param[];
>  	initcall_t *fn;
>  
> -	strcpy(static_command_line, saved_command_line);
> +	strcpy(tmp_cmdline, saved_command_line);
>  	parse_args(initcall_level_names[level],
> -		   static_command_line, __start___param,
> +		   tmp_cmdline, __start___param,
>  		   __stop___param - __start___param,
>  		   level, level,
>  		   &repair_env_string);

Hold on. static_command_line can be only accessed within init/main.c. As
far as I can say, it is only used by unknown_bootoption() (so your
__setup callbacks) and then in the do_initcall_level().

So, assuming that it is actually legal to modify static_command_line in
__setup()-s (and I must say I have rather mixed feelings about it ;-),
the only place that will be able to make use of this changes will be
do_initcall_level().

But your change actually uses *saved*_command_line instead of
*static*_command_line as the base for parse_args.

Generally I agree that the commit in question changed the semantics in a
subtle way - it makes the do_initcalls() use saved_command_line as a
string to be parsed instead of static_command_line. I was convinced that
at this stage of execution they must be identical (and the
static_command_line is a de-facto scratch buffer). You're saying that is
may not be the case, which can be true, but you're keeping the same
behaviour :-)

So either you have some extra changes in your kernel actually using
static_command_line for some other reason, or your change makes no
difference. The third option is me being brain dead today, which is not
impossible ;-)

Paweł

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/