[PATCH] x86: Parse CONFIG_CMDLINE in compressed kernel

[PATCH] x86: Parse CONFIG_CMDLINE in compressed kernel

[Baskov Evgeniy]

CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE was 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.

Parse CONFIG_CMDLINE-related options correctly in compressed
kernel code.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..dd8cbbe61dff 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -22,9 +22,49 @@ unsigned long get_cmd_line_ptr(void)
 }
 int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
-	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
+	int len = -1;
+	unsigned long cmdline_ptr;
+
+	/*
+	 * First try command line string provided by user,
+	 * then try command line string configured at comple time.
+	 * Skip first step if CONFIG_CMDLINE_OVERRIDE is enabled
+	 * and parse only compile time command line.
+	 */
+
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+		cmdline_ptr = get_cmd_line_ptr();
+		len = __cmdline_find_option(cmdline_ptr, option,
+					    buffer, bufsize);
+	}
+
+#ifdef CONFIG_CMDLINE_BOOL
+	if (len < 0) {
+		cmdline_ptr = (unsigned long)CONFIG_CMDLINE;
+		len = __cmdline_find_option(cmdline_ptr, option,
+					    buffer, bufsize);
+	}
+#endif
+
+	return len;
 }
 int cmdline_find_option_bool(const char *option)
 {
-	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
+	int len = -1;
+	unsigned long cmdline_ptr;
+
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+		cmdline_ptr = get_cmd_line_ptr();
+		len = __cmdline_find_option_bool(cmdline_ptr, option);
+	}
+
+
+#ifdef CONFIG_CMDLINE_BOOL
+	if (len < 0) {
+		cmdline_ptr = (unsigned long)CONFIG_CMDLINE;
+		len = __cmdline_find_option_bool(cmdline_ptr, option);
+	}
+#endif
+
+	return len;
 }
-- 
2.35.1

Re: [PATCH] x86: Parse CONFIG_CMDLINE in compressed kernel

[Borislav Petkov]

On Thu, Apr 07, 2022 at 05:40:14AM +0300, Baskov Evgeniy wrote:
> CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE was 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.
> 
> Parse CONFIG_CMDLINE-related options correctly in compressed
> kernel code.
> 
> Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>
> 
> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
> index f1add5d85da9..dd8cbbe61dff 100644
> --- a/arch/x86/boot/compressed/cmdline.c
> +++ b/arch/x86/boot/compressed/cmdline.c
> @@ -22,9 +22,49 @@ unsigned long get_cmd_line_ptr(void)
>  }
>  int cmdline_find_option(const char *option, char *buffer, int bufsize)
>  {
> -	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
> +	int len = -1;
> +	unsigned long cmdline_ptr;
> +
> +	/*
> +	 * First try command line string provided by user,
> +	 * then try command line string configured at comple time.
> +	 * Skip first step if CONFIG_CMDLINE_OVERRIDE is enabled
> +	 * and parse only compile time command line.
> +	 */
> +
> +	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> +		cmdline_ptr = get_cmd_line_ptr();
> +		len = __cmdline_find_option(cmdline_ptr, option,
> +					    buffer, bufsize);
> +	}
> +
> +#ifdef CONFIG_CMDLINE_BOOL
> +	if (len < 0) {
> +		cmdline_ptr = (unsigned long)CONFIG_CMDLINE;
> +		len = __cmdline_find_option(cmdline_ptr, option,
> +					    buffer, bufsize);
> +	}
> +#endif

Do I see it correctly that all this logic boils down to returning the
proper cmdline ptr and so you can do that once in get_cmd_line_ptr()
instead of duplicating the ifdeffery?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86: Parse CONFIG_CMDLINE in compressed kernel

[baskov]

On 2022-05-04 11:59, Borislav Petkov wrote:
> On Thu, Apr 07, 2022 at 05:40:14AM +0300, Baskov Evgeniy wrote:
>> CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE was 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.
>> 
>> Parse CONFIG_CMDLINE-related options correctly in compressed
>> kernel code.
>> 
>> Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>
>> 
>> diff --git a/arch/x86/boot/compressed/cmdline.c 
>> b/arch/x86/boot/compressed/cmdline.c
>> index f1add5d85da9..dd8cbbe61dff 100644
>> --- a/arch/x86/boot/compressed/cmdline.c
>> +++ b/arch/x86/boot/compressed/cmdline.c
>> @@ -22,9 +22,49 @@ unsigned long get_cmd_line_ptr(void)
>>  }
>>  int cmdline_find_option(const char *option, char *buffer, int 
>> bufsize)
>>  {
>> -	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, 
>> bufsize);
>> +	int len = -1;
>> +	unsigned long cmdline_ptr;
>> +
>> +	/*
>> +	 * First try command line string provided by user,
>> +	 * then try command line string configured at comple time.
>> +	 * Skip first step if CONFIG_CMDLINE_OVERRIDE is enabled
>> +	 * and parse only compile time command line.
>> +	 */
>> +
>> +	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
>> +		cmdline_ptr = get_cmd_line_ptr();
>> +		len = __cmdline_find_option(cmdline_ptr, option,
>> +					    buffer, bufsize);
>> +	}
>> +
>> +#ifdef CONFIG_CMDLINE_BOOL
>> +	if (len < 0) {
>> +		cmdline_ptr = (unsigned long)CONFIG_CMDLINE;
>> +		len = __cmdline_find_option(cmdline_ptr, option,
>> +					    buffer, bufsize);
>> +	}
>> +#endif
> 
> Do I see it correctly that all this logic boils down to returning the
> proper cmdline ptr and so you can do that once in get_cmd_line_ptr()
> instead of duplicating the ifdeffery?

Yes, it can be done by returning either concatenation of build-time
and run-time command line strings or only the build-time one,
depending on the kernel configuration.

Actually, it is implemented this way in arch/x86/kernel/setup.c.
I thought it's better to avoid imposing restrictions on kernel
command line length, but since it is already done, I guess,
I will do the same way in v2.

Thanks,
Baskov Evgeniy

[PATCH v2 0/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[Baskov Evgeniy]

CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE was 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.

v2 changes:

As Borislav Petkov stated, we can do the work just once and then
return correct command line pointer. Doing it this way require
us to perform string manipulations, so the first patch adds missing
strlcat() to compressed kernel, since this function simplifies
the code a lot.

If we need to concatenate strings, static buffer of fixed length
is used. The maximum command line length is set to the same value
as the one in the kernel setup code.

Baskov Evgeniy (2):
         x86: add strlcat() to compressed kernel
         x86: Parse CONFIG_CMDLINE in compressed kernel

 arch/x86/boot/compressed/cmdline.c | 28 ++++++++++++++++++++++++++--
 arch/x86/boot/compressed/string.c  | 15 +++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

[PATCH v2 1/2] x86: add strlcat() to compressed kernel

[Baskov Evgeniy]

strlcat() simplifies the code of command line
concatenation and reduces the probability of mistakes.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1eaa3229..b0635539b6f6 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -40,6 +40,21 @@ 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;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count-1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
 void *memset(void *s, int c, size_t n)
 {
 	int i;
-- 
2.36.0

[PATCH v2 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[Baskov Evgeniy]

CONFIG_CMDLINE, CONFIG_CMDLINE_BOOL, and CONFIG_CMDLINE_OVERRIDE were
ignored during options lookup in compressed kernel.

Parse CONFIG_CMDLINE-related options correctly in compressed kernel
code.

cmd_line_ptr is explicitly placed in .data section since it is used
and expected to be equal to zero before .bss section is cleared.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..58723983933d 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,22 +1,46 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "misc.h"
 
+#define COMMAND_LINE_SIZE 2048
+
 static unsigned long fs;
+
 static inline void set_fs(unsigned long seg)
 {
 	fs = seg << 4;  /* shift it back */
 }
+
 typedef unsigned long addr_t;
+
 static inline char rdfs8(addr_t addr)
 {
 	return *((char *)(fs + addr));
 }
+
 #include "../cmdline.c"
+
+static unsigned long cmd_line_ptr __section(".data");
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
+#endif
+
 unsigned long get_cmd_line_ptr(void)
 {
-	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
+	if (!cmd_line_ptr) {
+		cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
+
+		cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
+
+#ifdef CONFIG_CMDLINE_BOOL
+		if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+			strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+			strlcat(builtin_cmdline,
+				(char *)cmd_line_ptr, COMMAND_LINE_SIZE);
+		}
 
-	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
+		cmd_line_ptr = (unsigned long)builtin_cmdline;
+#endif
+	}
 
 	return cmd_line_ptr;
 }
-- 
2.36.0

[PATCH v3 0/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[Baskov Evgeniy]

CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE was 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.

v2 changes:

As Borislav Petkov stated, we can do the work just once and then
return correct command line pointer. Doing it this way require
us to perform string manipulations, so the first patch adds missing
strlcat() to compressed kernel, since this function simplifies
the code a lot.

If we need to concatenate strings, static buffer of fixed length
is used. The maximum command line length is set to the same value
as the one in the kernel setup code.

v3 changes:

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.
It was fixed by replacing the pointer by a boolean variable.

Baskov Evgeniy (2):
         x86: Add strlcat() to compressed kernel
         x86: Parse CONFIG_CMDLINE in compressed kernel

 arch/x86/boot/compressed/cmdline.c | 24 +++++++++++++++++++++++-
 arch/x86/boot/compressed/string.c  | 15 +++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

[PATCH v3 1/2] x86: Add strlcat() to compressed kernel

[Baskov Evgeniy]

strlcat() simplifies the code of command line
concatenation and reduces the probability of mistakes.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1eaa3229..b0635539b6f6 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -40,6 +40,21 @@ 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;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count-1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
 void *memset(void *s, int c, size_t n)
 {
 	int i;
-- 
2.36.0

Re: [PATCH v3 1/2] x86: Add strlcat() to compressed kernel

[Borislav Petkov]

On Thu, May 05, 2022 at 01:32:23PM +0300, Baskov Evgeniy wrote:
> Subject: Re: [PATCH v3 1/2] x86: Add strlcat() to compressed kernel

The tip tree preferred format for patch subject prefixes is
'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
'genirq/core:'. Please do not use file names or complete file paths as
prefix. 'git log path/to/file' should give you a reasonable hint in most
cases.

The condensed patch description in the subject line should start with a
uppercase letter and should be written in imperative tone.

In your case, that would be x86/boot: Add...

> strlcat() simplifies the code of command line
> concatenation and reduces the probability of mistakes.
> 
> Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>
> 
> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> index 81fc1eaa3229..b0635539b6f6 100644
> --- a/arch/x86/boot/compressed/string.c
> +++ b/arch/x86/boot/compressed/string.c
> @@ -40,6 +40,21 @@ 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;

You can add the BUG_ON() check from the kernel proper version like this:

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index b0635539b6f6..643fcd957527 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -46,6 +46,10 @@ size_t strlcat(char *dest, const char *src, size_t count)
 	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)
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) {}

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 1/2] x86: Add strlcat() to compressed kernel

[baskov]

Sorry for delayed reply.

On 2022-05-12 14:10, Borislav Petkov wrote:
> On Thu, May 05, 2022 at 01:32:23PM +0300, Baskov Evgeniy wrote:
>> Subject: Re: [PATCH v3 1/2] x86: Add strlcat() to compressed kernel
> 
> The tip tree preferred format for patch subject prefixes is
> 'subsys/component:', e.g. 'x86/apic:', 'x86/mm/fault:', 'sched/fair:',
> 'genirq/core:'. Please do not use file names or complete file paths as
> prefix. 'git log path/to/file' should give you a reasonable hint in 
> most
> cases.
> 
> The condensed patch description in the subject line should start with a
> uppercase letter and should be written in imperative tone.
> 
> In your case, that would be x86/boot: Add...

Thank you, I'll fix that in v4.

> 
> You can add the BUG_ON() check from the kernel proper version like 
> this:
> 
> diff --git a/arch/x86/boot/compressed/string.c
> b/arch/x86/boot/compressed/string.c
> index b0635539b6f6..643fcd957527 100644
> --- a/arch/x86/boot/compressed/string.c
> +++ b/arch/x86/boot/compressed/string.c
> @@ -46,6 +46,10 @@ size_t strlcat(char *dest, const char *src, size_t 
> count)
>  	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)
> 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) {}

Ok, will do.

[PATCH v3 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[Baskov Evgeniy]

CONFIG_CMDLINE, CONFIG_CMDLINE_BOOL, and CONFIG_CMDLINE_OVERRIDE were
ignored during options lookup in compressed kernel.

Parse CONFIG_CMDLINE-related options correctly in compressed kernel
code.

cmd_line_ptr_init is explicitly placed in .data section since it is
used and expected to be equal to zero before .bss section is cleared.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..261f53ad395a 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "misc.h"
 
+#define COMMAND_LINE_SIZE 2048
+
 static unsigned long fs;
 static inline void set_fs(unsigned long seg)
 {
@@ -12,12 +14,32 @@ static inline char rdfs8(addr_t addr)
 	return *((char *)(fs + addr));
 }
 #include "../cmdline.c"
+
+#ifdef CONFIG_CMDLINE_BOOL
+static char builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
+static bool builtin_cmdline_init __section(".data");
+#endif
+
 unsigned long get_cmd_line_ptr(void)
 {
 	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
-
 	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
 
+#ifdef CONFIG_CMDLINE_BOOL
+	if (!builtin_cmdline_init) {
+		if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+			strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+			strlcat(builtin_cmdline,
+				(char *)cmd_line_ptr,
+				COMMAND_LINE_SIZE);
+		}
+
+		builtin_cmdline_init = 1;
+	}
+
+	cmd_line_ptr = (unsigned long)builtin_cmdline;
+#endif
+
 	return cmd_line_ptr;
 }
 int cmdline_find_option(const char *option, char *buffer, int bufsize)
-- 
2.36.0

Re: [PATCH v3 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[Borislav Petkov]

On Thu, May 05, 2022 at 01:32:24PM +0300, Baskov Evgeniy wrote:

Same note on the subject format as for your previous patch.

> CONFIG_CMDLINE, CONFIG_CMDLINE_BOOL, and CONFIG_CMDLINE_OVERRIDE were
> ignored during options lookup in compressed kernel.
> 
> Parse CONFIG_CMDLINE-related options correctly in compressed kernel
> code.
> 
> cmd_line_ptr_init is explicitly placed in .data section since it is
> used and expected to be equal to zero before .bss section is cleared.

What I'm missing in this commit message is the use case which you have
in your 0/2 mail.

Also, to the tone of your commit messages, from
Documentation/process/submitting-patches.rst:

 "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour."

Also, do not talk about what your patch does - that should hopefully be
visible in the diff itself. Rather, talk about *why* you're doing what
you're doing.

> Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>
> 
> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
> index f1add5d85da9..261f53ad395a 100644
> --- a/arch/x86/boot/compressed/cmdline.c
> +++ b/arch/x86/boot/compressed/cmdline.c
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "misc.h"
>  
> +#define COMMAND_LINE_SIZE 2048
> +
>  static unsigned long fs;
>  static inline void set_fs(unsigned long seg)
>  {
> @@ -12,12 +14,32 @@ static inline char rdfs8(addr_t addr)
>  	return *((char *)(fs + addr));
>  }
>  #include "../cmdline.c"
> +
> +#ifdef CONFIG_CMDLINE_BOOL
> +static char builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
> +static bool builtin_cmdline_init __section(".data");
> +#endif
> +
>  unsigned long get_cmd_line_ptr(void)
>  {
>  	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
> -
>  	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
>  
> +#ifdef CONFIG_CMDLINE_BOOL
> +	if (!builtin_cmdline_init) {
> +		if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> +			strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +			strlcat(builtin_cmdline,
> +				(char *)cmd_line_ptr,
> +				COMMAND_LINE_SIZE);
> +		}
> +
> +		builtin_cmdline_init = 1;
> +	}
> +
> +	cmd_line_ptr = (unsigned long)builtin_cmdline;
> +#endif

I had asked this already but let me try again: instead of copying this
from kernel proper, why don't you add a common helper which you call
from both locations?

And it is not like this is going to be a huge function so you can stick
it into a shared header in arch/x86/include/asm/shared/ and it'll get
inlined into both locations...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[baskov]

On 2022-05-12 14:21, Borislav Petkov wrote:
> On Thu, May 05, 2022 at 01:32:24PM +0300, Baskov Evgeniy wrote:
> 
> Same note on the subject format as for your previous patch.
> 

Thanks.

>> CONFIG_CMDLINE, CONFIG_CMDLINE_BOOL, and CONFIG_CMDLINE_OVERRIDE were
>> ignored during options lookup in compressed kernel.
>> 
>> Parse CONFIG_CMDLINE-related options correctly in compressed kernel
>> code.
>> 
>> cmd_line_ptr_init is explicitly placed in .data section since it is
>> used and expected to be equal to zero before .bss section is cleared.
> 
> What I'm missing in this commit message is the use case which you have
> in your 0/2 mail.
> 
> Also, to the tone of your commit messages, from
> Documentation/process/submitting-patches.rst:
> 
>  "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>   instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>   to do frotz", as if you are giving orders to the codebase to change
>   its behaviour."
> 
> Also, do not talk about what your patch does - that should hopefully be
> visible in the diff itself. Rather, talk about *why* you're doing what
> you're doing.
> 

Will fix.

> 
> I had asked this already but let me try again: instead of copying this
> from kernel proper, why don't you add a common helper which you call
> from both locations?
> 
> And it is not like this is going to be a huge function so you can stick
> it into a shared header in arch/x86/include/asm/shared/ and it'll get
> inlined into both locations...

Oh, now I got what you meant, I'll factor that out in the next version.
There are currently no arch/x86/include/asm/shared/ directory,
so, I guess, it will be OK to put the header just in
arch/x86/include/asm/?

--
Baskov Evgeniy

Re: [PATCH v3 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[Borislav Petkov]

On Wed, May 25, 2022 at 08:25:30AM +0300, baskov@ispras.ru wrote:
> There are currently no arch/x86/include/asm/shared/ directory,

There is now - it was in tip but wandered upstream two days ago. :)

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 2/2] x86: Parse CONFIG_CMDLINE in compressed kernel

[baskov]

On 2022-05-25 11:22, Borislav Petkov wrote:
> On Wed, May 25, 2022 at 08:25:30AM +0300, baskov@ispras.ru wrote:
>> There are currently no arch/x86/include/asm/shared/ directory,
> 
> There is now - it was in tip but wandered upstream two days ago. :)
> 
> Thx.

Oh, thanks, I'd better rebase then...

--
Baskov Evgeniy

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

Evgeniy Baskov (5):
  x86/boot: Add strlcat() to compressed kernel
  x86: Add resolve_cmdline() helper
  x86/setup: Use resolve_cmdline() in setup.c
  x86/boot: Use resolve_cmdline() 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           | 19 +++++++++++
 arch/x86/include/asm/shared/setup-cmdline.h | 38 +++++++++++++++++++++
 arch/x86/kernel/setup.c                     | 22 +++---------
 arch/x86/purgatory/purgatory.c              |  1 +
 8 files changed, 85 insertions(+), 28 deletions(-)
 create mode 100644 arch/x86/include/asm/shared/setup-cmdline.h

-- 
2.36.1

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

[Evgeniy Baskov]

strlcat() simplifies the code of command line
concatenation and reduces the probability of mistakes.

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1eaa3229..6d74d31bf3d9 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -40,6 +40,25 @@ 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;
+}
+
 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.36.1

[PATCH v4 2/5] x86: Add resolve_cmdline() 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>

 create mode 100644 arch/x86/include/asm/shared/setup-cmdline.h

diff --git a/arch/x86/include/asm/shared/setup-cmdline.h b/arch/x86/include/asm/shared/setup-cmdline.h
new file mode 100644
index 000000000000..9822e5af4925
--- /dev/null
+++ b/arch/x86/include/asm/shared/setup-cmdline.h
@@ -0,0 +1,38 @@
+/* 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 resolve_cmdline(char *command_line,
+				   const char *boot_command_line)
+{
+#ifdef CONFIG_CMDLINE_BOOL
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+		/* Append boot loader cmdline to builtin */
+		strlcat(command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	}
+#else
+	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+#endif
+}
+
+#endif /* _ASM_X86_SETUP_CMDLINE_H */
-- 
2.36.1

Re: [PATCH v4 2/5] x86: Add resolve_cmdline() helper

[Borislav Petkov]

On Wed, May 25, 2022 at 01:10:10PM +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>
> 

You have some weird configuration to your git send-email which doesn't
add the "---" to split the patch commit message from the diffstat.

>  create mode 100644 arch/x86/include/asm/shared/setup-cmdline.h
> 
> diff --git a/arch/x86/include/asm/shared/setup-cmdline.h b/arch/x86/include/asm/shared/setup-cmdline.h

Just cmdline.h I'd say.

> new file mode 100644
> index 000000000000..9822e5af4925
> --- /dev/null
> +++ b/arch/x86/include/asm/shared/setup-cmdline.h
> @@ -0,0 +1,38 @@
> +/* 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.
> + */
> +


^ Superfluous newline.

> +static inline void resolve_cmdline(char *command_line,
> +				   const char *boot_command_line)

cmdline_prepare() I'd say.

> +{
> +#ifdef CONFIG_CMDLINE_BOOL
> +	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> +		/* Append boot loader cmdline to builtin */
> +		strlcat(command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE);
> +	}
> +#else
> +	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);

So that has been switched to strscpy() in the meantime:

8a33d96bd178 ("x86/setup: Use strscpy() to replace deprecated strlcpy()")

Please redo your set ontop of latest tip/master.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 2/5] x86: Add resolve_cmdline() helper

[Evgeniy Baskov]

On 2022-08-26 14:35, Borislav Petkov wrote:
> On Wed, May 25, 2022 at 01:10:10PM +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>
>> 
> 
> You have some weird configuration to your git send-email which doesn't
> add the "---" to split the patch commit message from the diffstat.

Thanks, apparently "--summary" removes that.

...
> 
> So that has been switched to strscpy() in the meantime:
> 
> 8a33d96bd178 ("x86/setup: Use strscpy() to replace deprecated 
> strlcpy()")

I did already replace strlcpy() in v5, but since there are new changes
needed, I will send v6 with those changes applied soon.
So, please, ignore v5 then.

> 
> Please redo your set ontop of latest tip/master.

Will do.

Thanks,
Evgeniy Baskov

[PATCH v4 3/5] x86/setup: Use resolve_cmdline() 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>

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 249981bf3d8a..880465e26da2 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -47,6 +47,7 @@
 #include <asm/pci-direct.h>
 #include <asm/prom.h>
 #include <asm/proto.h>
+#include <asm/shared/setup-cmdline.h>
 #include <asm/thermal.h>
 #include <asm/unwind.h>
 #include <asm/vsyscall.h>
@@ -164,10 +165,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;
@@ -901,20 +899,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
-	strlcpy(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);
-		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-	}
-#endif
-#endif
-
-	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	resolve_cmdline(command_line, boot_command_line);
+	strlcpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
 	/*
-- 
2.36.1

[PATCH v4 4/5] x86/boot: Use resolve_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.

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>

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..77a758146531 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) {
+		resolve_cmdline(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 4910bf230d7b..e30159dc81b4 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/setup-cmdline.h>
 
 #include "tdx.h"
 
-- 
2.36.1

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

[Evgeniy Baskov]

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

Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 44c350d627c7..7dc0300529fa 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.36.1