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

[PATCH v7 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.

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                 | 22 ++---------
 arch/x86/purgatory/purgatory.c          |  1 +
 8 files changed, 113 insertions(+), 28 deletions(-)
 create mode 100644 arch/x86/include/asm/shared/cmdline.h

-- 
2.35.1

[PATCH v7 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.35.1

[PATCH v7 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..32d4a792a38a
--- /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 COMMAND_LINE_INIT CONFIG_CMDLINE
+#else
+#define COMMAND_LINE_INIT ""
+#endif
+
+/*
+ * command_line and boot_command_line are expected to be at most
+ * COMMAND_LINE_SIZE length. command_line needs to be initialized
+ * with COMMAND_LINE_INIT.
+ */
+static inline void cmdline_prepare(char *command_line,
+				   const char *boot_command_line)
+{
+	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
+		strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	} else if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+		/* Append boot loader cmdline to builtin one. */
+		strlcat(command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	}
+}
+
+#endif /* _ASM_X86_SETUP_CMDLINE_H */
-- 
2.35.1

[PATCH v7 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 | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7144ee..1acea005cad6 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -49,6 +49,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>
@@ -166,10 +167,7 @@ unsigned long saved_video_mode;
 #define RAMDISK_PROMPT_FLAG		0x8000
 #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
+static char command_line[COMMAND_LINE_SIZE] __initdata = COMMAND_LINE_INIT;
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
 struct edd edd;
@@ -969,20 +967,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.35.1

[PATCH v7 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..717f450662aa 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") = COMMAND_LINE_INIT;
+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.35.1

[PATCH v7 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 4a3f223973f4..39e455c105a7 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -31,10 +31,6 @@
 #include <linux/ctype.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.35.1

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

[Borislav Petkov]

On Tue, Oct 04, 2022 at 03:48:21PM +0300, Evgeniy Baskov wrote:
> +static inline void cmdline_prepare(char *command_line,
> +				   const char *boot_command_line)
> +{
> +	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
> +		strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> +	} else if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> +		/* Append boot loader cmdline to builtin one. */
> +		strlcat(command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE);
> +	}

Why can't we make this plain and simple and understandable at a quick
glance instead of putting parts of the string in some variable and then
still doing copying outside of the function?

IOW, simply put everything in a single function:

static inline void cmdline_prepare(char *dst,
                                   const char *builtin_cmdline,
                                   const 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]) {
                        /* append boot loader cmdline to builtin */
                        srtlcat(dst, builtin_cmdline, COMMAND_LINE_SIZE)
                        strlcat(dst, " ", COMMAND_LINE_SIZE);
                        strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
                } else {
                        strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
                }
        }
}

which is then called like this:

	cmdline_prepare(command_line, builtin_cmdline, boot_command_line);

and when it returns command_line is ready. And before it, command_line
is the empty string:

static char __initdata command_line[COMMAND_LINE_SIZE];

And all the complexity is in one single function and that single
function does each case in an obvious manner and you don't have to go
look outside of the function body to understand what it does.

Hmmm?

-- 
Regards/Gruss,
    Boris.

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

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

[Evgeniy Baskov]

On 2022-11-10 01:54, Borislav Petkov wrote:
> On Tue, Oct 04, 2022 at 03:48:21PM +0300, Evgeniy Baskov wrote:
>> +static inline void cmdline_prepare(char *command_line,
>> +				   const char *boot_command_line)
>> +{
>> +	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
>> +		strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> +	} else if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
>> +		/* Append boot loader cmdline to builtin one. */
>> +		strlcat(command_line, " ", COMMAND_LINE_SIZE);
>> +		strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> +	}
> 
> Why can't we make this plain and simple and understandable at a quick
> glance instead of putting parts of the string in some variable and then
> still doing copying outside of the function?
> 
> IOW, simply put everything in a single function:
> 
> static inline void cmdline_prepare(char *dst,
>                                    const char *builtin_cmdline,
>                                    const 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]) {
>                         /* append boot loader cmdline to builtin */
>                         srtlcat(dst, builtin_cmdline, 
> COMMAND_LINE_SIZE)
>                         strlcat(dst, " ", COMMAND_LINE_SIZE);
>                         strlcat(dst, boot_command_line, 
> COMMAND_LINE_SIZE);
>                 } else {
>                         strscpy(dst, boot_command_line, 
> COMMAND_LINE_SIZE);
>                 }
>         }
> }
> 
> which is then called like this:
> 
> 	cmdline_prepare(command_line, builtin_cmdline, boot_command_line);
> 
> and when it returns command_line is ready. And before it, command_line
> is the empty string:
> 
> static char __initdata command_line[COMMAND_LINE_SIZE];
> 
> And all the complexity is in one single function and that single
> function does each case in an obvious manner and you don't have to go
> look outside of the function body to understand what it does.
> 
> Hmmm?

Well, my version takes one less copy... But clarity is more important
in the kernel, I guess. I'll send updated version soon.

Thanks,
Evgeniy Baskov

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

[Borislav Petkov]

On Thu, Nov 10, 2022 at 11:59:49AM +0300, Evgeniy Baskov wrote:
> Well, my version takes one less copy... But clarity is more important
> in the kernel, I guess.

Yap, you guessed right.

I'd prefer to have the code plain and simple and easy to "reload" into
my L1 instead of having to parse it again in months, years from now.

Thx.

-- 
Regards/Gruss,
    Boris.

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