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

[Thomas Gleixner]

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@xxxxxxxxx>
> ---
>  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