[PATCH v8 0/5] Parse CONFIG_CMDLINE in compressed kernel

[PATCH v8 0/5] Parse CONFIG_CMDLINE in compressed kernel

[Evgeniy Baskov]

CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE were ignored during
options lookup in compressed kernel, including earlyprintk option,
so it was impossible to get earlyprintk messages from that stage
of boot process via command line provided at compile time.
Being able to enable earlyprintk via compile-time option might
be desirable for booting on systems with broken UEFI command line
arguments via EFISTUB.

Changes in v2:

* Compute resulting cmdline string once if needed and then reuse it.
  Store concatenation result in a static buffer.
* Add strlcat() to compressed kernel to simplify the code.

Changes in v3:

v2 had a bug: cmd_line_ptr was set to a pointer to a buffer inside
a kernel before kernel relocation, that makes this pointer invalid.

* Replace the pointer by a boolean variable to avoid storing a pointer,
  since it becomes invalid during kernel relocation.

Changes in v4:

* Use better wording for commit messages.
* Add buffer overflow check to strlcat().
* Factor out common logic of cmdline resolving into helper function.

Changes in v5:

* Use strscpy() instead of strlcpy().

Changes in v6:

* Remove superfluous new line.
* Rename resolve_cmdline() to cmdline_prepare().
* Move shared/setup-cmdline.h to shared/cmdline.h

Changes in v7:

* Replace #ifdef with IS_ENABLED() in cmdline_prepare()
  for consistency.

Changes in v8:

* Remove the need to use special initial value for command line result
  variable and put all logic in one function to improve the clarity
  of the code.

One #ifdef is still required for the initial value of builtin cmdline
in case iti is not defined to enable the use of IS_ENABLED in
cmdline_prapare(), since it is more readable.

Evgeniy Baskov (5):
  x86/boot: Add strlcat() and strscpy() to compressed kernel
  x86: Add cmdline_prepare() helper
  x86/setup: Use cmdline_prepare() in setup.c
  x86/boot: Use cmdline_prapare() in compressed kernel
  x86/boot: Remove no longer needed includes

 arch/x86/boot/compressed/cmdline.c      | 24 +++++++++++-
 arch/x86/boot/compressed/ident_map_64.c |  4 --
 arch/x86/boot/compressed/kaslr.c        |  4 --
 arch/x86/boot/compressed/misc.h         |  1 +
 arch/x86/boot/compressed/string.c       | 50 +++++++++++++++++++++++++
 arch/x86/include/asm/shared/cmdline.h   | 35 +++++++++++++++++
 arch/x86/kernel/setup.c                 | 20 ++--------
 arch/x86/purgatory/purgatory.c          |  1 +
 8 files changed, 112 insertions(+), 27 deletions(-)
 create mode 100644 arch/x86/include/asm/shared/cmdline.h

-- 
2.37.4

[PATCH v8 1/5] x86/boot: Add strlcat() and strscpy() to compressed kernel

[Evgeniy Baskov]

These functions simplify the code of command line concatenation
helper and reduce the probability of mistakes.

Use simpler implementation of strscpy than used in it kernel itself
to avoid code bloat in compressed kernel.

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/boot/compressed/string.c | 50 +++++++++++++++++++++++++++++++
 arch/x86/purgatory/purgatory.c    |  1 +
 2 files changed, 51 insertions(+)

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1eaa3229..5c193fa0a09b 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -40,6 +40,56 @@ static void *____memcpy(void *dest, const void *src, size_t n)
 }
 #endif
 
+size_t strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = strlen(dest);
+	size_t len = strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		error("strlcat(): destination too big\n");
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count-1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
+/* Don't include word-at-a-time code path in compressed kernel for simplicity */
+size_t strscpy(char *dest, const char *src, size_t count)
+{
+	long res = 0;
+
+	if (count == 0)
+		return -E2BIG;
+
+	if (count > INT_MAX) {
+		warn("strscpy(): Count is too big");
+		return -E2BIG;
+	}
+
+	while (count) {
+		char c;
+
+		c = src[res];
+		dest[res] = c;
+		if (!c)
+			return res;
+		res++;
+		count--;
+	}
+
+	/* Hit buffer length without finding a NUL; force NUL-termination. */
+	if (res)
+		dest[res-1] = '\0';
+
+	return -E2BIG;
+}
+
 void *memset(void *s, int c, size_t n)
 {
 	int i;
diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
index 7558139920f8..65f0cedb65ae 100644
--- a/arch/x86/purgatory/purgatory.c
+++ b/arch/x86/purgatory/purgatory.c
@@ -57,3 +57,4 @@ void purgatory(void)
  * arch/x86/boot/compressed/string.c
  */
 void warn(const char *msg) {}
+void error(char *m) {}
-- 
2.37.4

[PATCH v8 2/5] x86: Add cmdline_prepare() helper

[Evgeniy Baskov]

Command line needs to be combined in both compressed and uncompressed
kernel from built-in and boot command line strings, which requires
non-trivial logic depending on CONFIG_CMDLINE_BOOL and
CONFIG_CMDLINE_OVERRIDE.

Add a helper function to avoid code duplication.

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/include/asm/shared/cmdline.h | 35 +++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 arch/x86/include/asm/shared/cmdline.h

diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
new file mode 100644
index 000000000000..01736d66028d
--- /dev/null
+++ b/arch/x86/include/asm/shared/cmdline.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_X86_SETUP_CMDLINE_H
+#define _ASM_X86_SETUP_CMDLINE_H
+
+#define _SETUP
+#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
+#include <linux/string.h>
+
+#ifdef CONFIG_CMDLINE_BOOL
+#define BUILTIN_COMMAND_LINE CONFIG_CMDLINE
+#else
+#define BUILTIN_COMMAND_LINE ""
+#endif
+
+static inline void cmdline_prepare(char *dst, const char *boot_command_line)
+{
+	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
+		strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
+	} else {
+		strscpy(dst, BUILTIN_COMMAND_LINE, COMMAND_LINE_SIZE);
+		/*
+		 * Append boot loader cmdline to builtin, if it exists
+		 * and should not be overriden.
+		 */
+		if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) && boot_command_line[0]) {
+			strlcat(dst, " ", COMMAND_LINE_SIZE);
+			strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
+		}
+	}
+}
+
+#endif /* _ASM_X86_SETUP_CMDLINE_H */
-- 
2.37.4

[PATCH v8 3/5] x86/setup: Use cmdline_prepare() in setup.c

[Evgeniy Baskov]

Use a common helper function for command line resolving in
arch/x86/kernel/setup.c for code unification.

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/kernel/setup.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index aacaa96f0195..f23a49f62b71 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -50,6 +50,7 @@
 #include <asm/pci-direct.h>
 #include <asm/prom.h>
 #include <asm/proto.h>
+#include <asm/shared/cmdline.h>
 #include <asm/thermal.h>
 #include <asm/unwind.h>
 #include <asm/vsyscall.h>
@@ -168,9 +169,6 @@ unsigned long saved_video_mode;
 #define RAMDISK_LOAD_FLAG		0x4000
 
 static char __initdata command_line[COMMAND_LINE_SIZE];
-#ifdef CONFIG_CMDLINE_BOOL
-static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
-#endif
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
 struct edd edd;
@@ -970,20 +968,8 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.start = __pa_symbol(__bss_start);
 	bss_resource.end = __pa_symbol(__bss_stop)-1;
 
-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
-	strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
-	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);
-		strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-	}
-#endif
-#endif
-
-	strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	cmdline_prepare(command_line, boot_command_line);
+	strscpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
 	/*
-- 
2.37.4

[PATCH v8 4/5] x86/boot: Use cmdline_prapare() in compressed kernel

[Evgeniy Baskov]

CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE were ignored during
options lookup in compressed kernel, including earlyprintk option,
so it was impossible to get earlyprintk messages from that stage
of boot process via command line provided at compile time.
Being able to enable earlyprintk via compile-time option might
be desirable for booting on systems with broken UEFI command line
arguments via EFISTUB.

Use a common helper function to handle CONFIG_CMDLINE_* in compressed
kernel.

Place command_line_init explicitly in '.data' section since it is
used before '.bss' section is zeroed and it is expected to be equal
to zero.

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/boot/compressed/cmdline.c | 24 ++++++++++++++++++++++--
 arch/x86/boot/compressed/misc.h    |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..53ad259ee441 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -12,6 +12,15 @@ static inline char rdfs8(addr_t addr)
 	return *((char *)(fs + addr));
 }
 #include "../cmdline.c"
+
+static char command_line[COMMAND_LINE_SIZE] __section(".data");
+static bool command_line_init __section(".data");
+
+/*
+ * This always returns runtime command line and does not account for built-in
+ * command line, so this should not be used for cmdline parsing.
+ * Use get_cmdline() instead.
+ */
 unsigned long get_cmd_line_ptr(void)
 {
 	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
@@ -20,11 +29,22 @@ unsigned long get_cmd_line_ptr(void)
 
 	return cmd_line_ptr;
 }
+
+static inline unsigned long get_cmdline(void)
+{
+	if (!command_line_init) {
+		cmdline_prepare(command_line, (char *)get_cmd_line_ptr());
+		command_line_init = 1;
+	}
+
+	return (unsigned long)command_line;
+}
+
 int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
-	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
+	return __cmdline_find_option(get_cmdline(), option, buffer, bufsize);
 }
 int cmdline_find_option_bool(const char *option)
 {
-	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
+	return __cmdline_find_option_bool(get_cmdline(), option);
 }
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 62208ec04ca4..5bf1357c054c 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -26,6 +26,7 @@
 #include <asm/boot.h>
 #include <asm/bootparam.h>
 #include <asm/desc_defs.h>
+#include <asm/shared/cmdline.h>
 
 #include "tdx.h"
 
-- 
2.37.4

[PATCH v8 5/5] x86/boot: Remove no longer needed includes

[Evgeniy Baskov]

x86/incldue/asm/shared/cmdline.h header already provides
COMMAND_LINE_SIZE definition.

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 arch/x86/boot/compressed/ident_map_64.c | 4 ----
 arch/x86/boot/compressed/kaslr.c        | 4 ----
 2 files changed, 8 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index d4a314cc50d6..fdfe0e5f5cb0 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -33,10 +33,6 @@
 #define __PAGE_OFFSET __PAGE_OFFSET_BASE
 #include "../../mm/ident_map.c"
 
-#define _SETUP
-#include <asm/setup.h>	/* For COMMAND_LINE_SIZE */
-#undef _SETUP
-
 extern unsigned long get_cmd_line_ptr(void);
 
 /* Used by PAGE_KERN* macros: */
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index e476bcbd9b42..b73a4d45f8d7 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -32,10 +32,6 @@
 #include <generated/utsversion.h>
 #include <generated/utsrelease.h>
 
-#define _SETUP
-#include <asm/setup.h>	/* For COMMAND_LINE_SIZE */
-#undef _SETUP
-
 extern unsigned long get_cmd_line_ptr(void);
 
 /* Simplified build-specific string for starting entropy. */
-- 
2.37.4

Re: [PATCH v8 2/5] x86: Add cmdline_prepare() helper

[Borislav Petkov]

On Thu, Nov 10, 2022 at 04:09:30PM +0300, Evgeniy Baskov wrote:
> Command line needs to be combined in both compressed and uncompressed
> kernel from built-in and boot command line strings, which requires
> non-trivial logic depending on CONFIG_CMDLINE_BOOL and
> CONFIG_CMDLINE_OVERRIDE.
> 
> Add a helper function to avoid code duplication.
> 
> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> ---
>  arch/x86/include/asm/shared/cmdline.h | 35 +++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 arch/x86/include/asm/shared/cmdline.h
> 
> diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
> new file mode 100644
> index 000000000000..01736d66028d
> --- /dev/null
> +++ b/arch/x86/include/asm/shared/cmdline.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_X86_SETUP_CMDLINE_H
> +#define _ASM_X86_SETUP_CMDLINE_H
> +
> +#define _SETUP
> +#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
> +#undef _SETUP
> +
> +#include <linux/string.h>
> +
> +#ifdef CONFIG_CMDLINE_BOOL
> +#define BUILTIN_COMMAND_LINE CONFIG_CMDLINE
> +#else
> +#define BUILTIN_COMMAND_LINE ""
> +#endif
> +
> +static inline void cmdline_prepare(char *dst, const char *boot_command_line)
> +{
> +	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
> +		strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
> +	} else {
> +		strscpy(dst, BUILTIN_COMMAND_LINE, COMMAND_LINE_SIZE);
> +		/*
> +		 * Append boot loader cmdline to builtin, if it exists
> +		 * and should not be overriden.
> +		 */
> +		if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) && boot_command_line[0]) {
> +			strlcat(dst, " ", COMMAND_LINE_SIZE);
> +			strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
> +		}

You keep changing what I'm suggesting and the next patch has a strscpy()
outside of the function.

When I say it should be all concentrated in one function, I really mean
it.

So now it is my turn: I'll do it how I think it should be done and you
can review it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v8 2/5] x86: Add cmdline_prepare() helper

[Borislav Petkov]

On Mon, Nov 14, 2022 at 03:28:55PM +0100, Borislav Petkov wrote:
> So now it is my turn: I'll do it how I think it should be done and you
> can review it.

Ok, here are two patches as a reply to this message.

I was able to test them as much as I can in a VM here but I'd need more
details/testing in your configuration with earlyprintk as a builtin
cmdline.

cmdline_prepare() has grown a bit hairy in the end but I've tried hard
to comment what happens there so that it is clear for the future. The
main goal being to concentrate all command line strings processing in
that function and not have it spread around the tree. And yes, there are
more cleanups possible.

In the compressed stage I'm using the cmdline which is in boot_params as
source and destination to basically add only the builtin cmdline.

In kernel proper the boot_command_line comes from generic code and that
is a whole another way of crazy in itself when I look at init/main.c

And as previously stated - the goal is to have everything in one place
and documented as good as possible so that trying to figure out how
command line parsing is done doesn't send you on grepping spree around
the tree.

Suggestions how to simplify this even more are always welcome, ofc.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 1/2] x86/boot: Add a function for kernel command line preparation

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>
Date: Mon, 14 Nov 2022 15:31:59 +0100

Since both the compressed kernel and kernel proper need to deal with the
command line, add a common helper which abstracts away all the details.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/shared/cmdline.h | 34 +++++++++++++++++++++++++++
 arch/x86/kernel/setup.c               | 21 +++--------------
 2 files changed, 37 insertions(+), 18 deletions(-)
 create mode 100644 arch/x86/include/asm/shared/cmdline.h

diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
new file mode 100644
index 000000000000..e09c06338567
--- /dev/null
+++ b/arch/x86/include/asm/shared/cmdline.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_SHARED_CMDLINE_H
+#define _ASM_X86_SHARED_CMDLINE_H
+
+#ifdef CONFIG_CMDLINE_BOOL
+#define BUILTIN_CMDLINE CONFIG_CMDLINE
+#else
+#define BUILTIN_CMDLINE ""
+#endif
+
+static inline void cmdline_prepare(char *dst,
+                                   const char *builtin_cmdline,
+                                   char *boot_command_line)
+{
+	/* Depends on CONFIG_CMDLINE_BOOL, overwrite with builtin cmdline */
+	if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
+		strscpy(dst, builtin_cmdline, COMMAND_LINE_SIZE);
+	else if (IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
+		if (builtin_cmdline[0]) {
+			/* 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);
+		}
+	} else {
+		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);
+}
+
+#endif /* _ASM_X86_SHARED_CMDLINE_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index aacaa96f0195..c506807142a7 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -25,6 +25,7 @@
 #include <linux/static_call.h>
 #include <linux/swiotlb.h>
 #include <linux/random.h>
+#include <linux/vmalloc.h>
 
 #include <uapi/linux/mount.h>
 
@@ -50,10 +51,10 @@
 #include <asm/pci-direct.h>
 #include <asm/prom.h>
 #include <asm/proto.h>
+#include <asm/shared/cmdline.h>
 #include <asm/thermal.h>
 #include <asm/unwind.h>
 #include <asm/vsyscall.h>
-#include <linux/vmalloc.h>
 
 /*
  * max_low_pfn_mapped: highest directly mapped pfn < 4 GB
@@ -168,9 +169,6 @@ unsigned long saved_video_mode;
 #define RAMDISK_LOAD_FLAG		0x4000
 
 static char __initdata command_line[COMMAND_LINE_SIZE];
-#ifdef CONFIG_CMDLINE_BOOL
-static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
-#endif
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
 struct edd edd;
@@ -970,20 +968,7 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.start = __pa_symbol(__bss_start);
 	bss_resource.end = __pa_symbol(__bss_stop)-1;
 
-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
-	strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
-	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);
-		strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-	}
-#endif
-#endif
-
-	strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	cmdline_prepare(command_line, BUILTIN_CMDLINE, boot_command_line);
 	*cmdline_p = command_line;
 
 	/*
-- 
2.35.1


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 2/2] x86/boot: Use cmdline_prepare() in the compressed stage

[Borislav Petkov]

From: Borislav Petkov <bp@suse.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@ispras.ru>
Signed-off-by: Borislav Petkov <bp@suse.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



-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] x86/boot: Use cmdline_prepare() in the compressed stage

[Evgeniy Baskov]

On 2022-11-15 22:00, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.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@ispras.ru>
> Signed-off-by: Borislav Petkov <bp@suse.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