linux-kernel - Re: [PATCH 2/2] x86/boot: Use cmdline

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]
Message-ID: <d51808e14d76c5663d3eb946a92bce2d@ispras.ru>
Date:   Wed, 16 Nov 2022 13:48:48 +0300
From:   Evgeniy Baskov <baskov@...ras.ru>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Alexey Khoroshilov <khoroshilov@...ras.ru>
Subject: Re: [PATCH 2/2] x86/boot: Use cmdline_prepare() in the compressed
 stage

On 2022-11-15 22:00, Borislav Petkov wrote:
> From: Borislav Petkov <bp@...e.de>
> Date: Tue, 15 Nov 2022 19:30:09 +0100
> 
> Use cmdline_prepare() in the compressed stage so that builtin
> command line (CONFIG_CMDLINE_BOOL) and overridden command line
> (CONFIG_CMDLINE_OVERRIDE) strings are visible in the compressed kernel
> too.
> 
> Use case being, supplying earlyprintk via a compile-time option for
> booting on systems with broken UEFI command line arguments via EFISTUB.
> 
> Reported-by: Evgeniy Baskov <baskov@...ras.ru>
> Signed-off-by: Borislav Petkov <bp@...e.de>
> ---
>  arch/x86/boot/compressed/misc.c       |  7 +++++++
>  arch/x86/include/asm/shared/cmdline.h | 20 ++++++++++++++++----
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/misc.c 
> b/arch/x86/boot/compressed/misc.c
> index cf690d8712f4..b1077b2fdba6 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -18,6 +18,9 @@
>  #include "../string.h"
>  #include "../voffset.h"
>  #include <asm/bootparam_utils.h>
> +#include <asm/shared/cmdline.h>
> +
> +extern unsigned long get_cmd_line_ptr(void);
> 
>  /*
>   * WARNING!!
> @@ -355,6 +358,7 @@ asmlinkage __visible void *extract_kernel(void
> *rmode, memptr heap,
>  {
>  	const unsigned long kernel_total_size = VO__end - VO__text;
>  	unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
> +	char *cmdline = (char *)get_cmd_line_ptr();
>  	unsigned long needed_size;
> 
>  	/* Retain x86 boot parameters pointer passed from startup_32/64. */
> @@ -365,6 +369,9 @@ asmlinkage __visible void *extract_kernel(void
> *rmode, memptr heap,
> 
>  	sanitize_boot_params(boot_params);
> 
> +	/* Destination and boot command line are the same */
> +	cmdline_prepare(cmdline, BUILTIN_CMDLINE, cmdline);
> +
>  	if (boot_params->screen_info.orig_video_mode == 7) {
>  		vidmem = (char *) 0xb0000;
>  		vidport = 0x3b4;
> diff --git a/arch/x86/include/asm/shared/cmdline.h
> b/arch/x86/include/asm/shared/cmdline.h
> index e09c06338567..8a7d8f579575 100644
> --- a/arch/x86/include/asm/shared/cmdline.h
> +++ b/arch/x86/include/asm/shared/cmdline.h
> @@ -8,6 +8,15 @@
>  #define BUILTIN_CMDLINE ""
>  #endif
> 
> +#define _SETUP
> +#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
> +#undef _SETUP
> +
> +/*
> + * Add @boot_command_line to @dst only if it is not in @dst already.
> The compressed kernel
> + * has the command line pointer in setup_header.cmd_line_ptr which is
> set by the boot
> + * loader so @boot_command_line == @dst there, see the call in
> compressed/misc.c
> + */
>  static inline void cmdline_prepare(char *dst,
>                                     const char *builtin_cmdline,
>                                     char *boot_command_line)
> @@ -20,15 +29,18 @@ static inline void cmdline_prepare(char *dst,
>  			/* Add builtin cmdline */
>  			strlcat(dst, builtin_cmdline, COMMAND_LINE_SIZE);
>  			strlcat(dst, " ", COMMAND_LINE_SIZE);
> -			/* Add boot cmdline */
> -			strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
> +
> +			if (dst != boot_command_line)
> +				strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
>  		}
>  	} else {
> -		strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
> +		if (dst != boot_command_line)
> +			strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
>  	}
> 
>  	/* Copy back into boot command line, see setup_command_line() */
> -	strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
> +	if (dst != boot_command_line)
> +		strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
>  }
> 
>  #endif /* _ASM_X86_SHARED_CMDLINE_H */
> --
> 2.35.1


Thanks for your time!

This patch has a few problems I was trying to avoid though:
* It has different behavior for dst == boot_command_line and dst !=
   boot_command_line, since the order of parameters is different.
* It appends space at the end of command line, not as a separator.
* It also modifies boot_command_line in compressed kernel and it causes
   builtin command line to be appended twice.

First two problems would be fixed if compressed kernel used separate
buffer for modified cmdline like setup.c does. This also would simplify
the helper a bit and is required to fix the third problem.

The third problem was the reason I did not include the last strscpy() in
the helper. I still don't think it should be a part of the command line
preparation logic... If the code needs a copy for parse_early_param(),
it is related more to the parse_early_param() call, than to
cmdline_prepare().

Should I maybe make another iteration by removing lazy cmdline
initialization in compressed kernel and adding more comments?
I don't see though how the last strscpy() could cleanly fit into
cmdline_prepare().

Thanks,
Evgeniy Baskov