linux-kernel - Re: [PATCH] Staging: setup.c : boot loader kernel arguments are secured over cmdline.txt arguments for some systems

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:   Sat, 11 Feb 2017 21:06:03 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Kishore Karanala <kishore.karanala@...il.com>
cc:     tglx@...utronix.de, mingo@...hat.com,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        LKML <linux-kernel@...r.kernel.org>, matt@...eblueprint.co.uk,
        rafael.j.wysocki@...el.com, thgarnie@...gle.com,
        arbab@...ux.vnet.ibm.com, dave.hansen@...ux.intel.com
Subject: Re: [PATCH] Staging: setup.c : boot loader kernel arguments are
 secured over cmdline.txt arguments for some systems

On Sun, 12 Feb 2017, Kishore Karanala wrote:
1;2802;0c
The proper subsystem for x86 is x86 and not staging.

Also your subject line is a way too long sentence instead of a short and
precise summary of the change.

> boot loader kernel arguments are secured over cmdline.txt
> arguments for some systems

Repeating it does not make it any better.

What's missing here is an explanation WHY this changes is needed and which
problem it solves.

> Signed-off-by: Kishore Karanala <kishore.karanala@...il.com>
> ---
>  arch/x86/Kconfig        | 3 +++
>  arch/x86/kernel/setup.c | 7 ++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index e487493..3077fb0 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2224,6 +2224,9 @@ config CMDLINE
>  	  In most cases, the command line (whether built-in or provided
>  	  by the boot loader) should specify the device for the root
>  	  file system.
> +	 In some of the systems boot loader arguments needs dominated over
> +	 cmdline arguments in systems like automotive , this can be done using
> +	 CMDLINE="!root=/dev/mmcblk0p1 ro"

I really have no idea what that sentence means. Also it does not explain
what the exclamation mark stands for.

Aside of that the indentation of that paragraph is wrong.

>  config CMDLINE_OVERRIDE
>  	bool "Built-in command line overrides boot loader arguments"
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 4cfba94..5fa3194 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -962,11 +962,16 @@ void __init setup_arch(char **cmdline_p)
>  #ifdef CONFIG_CMDLINE_OVERRIDE
>  	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>  #else
> -	if (builtin_cmdline[0]) {
> +	if (builtin_cmdline[0] != '!') {
>  		/* append boot loader cmdline to builtin */
>  		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>  		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>  		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	} else {
> +		/* This will provide additional secuirty to cmdline */

I assume you mean security. What has this to do with security? The empty
changelog does not explain it neither does this comment.

> +		/* arguments not overriding bootloader arguments */

Multiline comments are formatted like this

		/*
		 * This is the first line of a multiline comment which
		 * continues on the second line.
		 */

> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, &builtin_cmdline[1], COMMAND_LINE_SIZE);

So this is just the reverse order of the exsiting mechanism. What's the
point of this exercise?

Thanks,

	tglx