| Message ID | 157867229521.17873.654222294326542349.stgit@devnote2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tracing: bootconfig: Boot-time tracing and Extra boot config | expand |
On Sat, Jan 11, 2020 at 01:04:55AM +0900, Masami Hiramatsu wrote: > Since the current kernel command line is too short to describe > long and many options for init (e.g. systemd command line options), > this allows admin to use boot config for init command line. > > All init command line under "init." keywords will be passed to > init. > > For example, > > init.systemd { > unified_cgroup_hierarchy = 1 > debug_shell > default_timeout_start_sec = 60 > } > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > --- > init/main.c | 31 ++++++++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/init/main.c b/init/main.c > index c0017d9d16e7..dd7da62d99a5 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -139,6 +139,8 @@ char *saved_command_line; > static char *static_command_line; > /* Untouched extra command line */ > static char *extra_command_line; > +/* Extra init arguments */ > +static char *extra_init_args; > > static char *execute_command; > static char *ramdisk_execute_command; > @@ -372,6 +374,8 @@ static void __init setup_boot_config(void) > pr_info("Load boot config: %d bytes\n", size); > /* keys starting with "kernel." are passed via cmdline */ > extra_command_line = xbc_make_cmdline("kernel"); > + /* Also, "init." keys are init arguments */ > + extra_init_args = xbc_make_cmdline("init"); > } > } > #else > @@ -507,16 +511,18 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { } > */ > static void __init setup_command_line(char *command_line) > { > - size_t len, xlen = 0; > + size_t len, xlen = 0, ilen = 0; > > if (extra_command_line) > xlen = strlen(extra_command_line); > + if (extra_init_args) > + ilen = strlen(extra_init_args) + 4; /* for " -- " */ > > len = xlen + strlen(boot_command_line) + 1; > > - saved_command_line = memblock_alloc(len, SMP_CACHE_BYTES); > + saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES); > if (!saved_command_line) > - panic("%s: Failed to allocate %zu bytes\n", __func__, len); > + panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen); > > static_command_line = memblock_alloc(len, SMP_CACHE_BYTES); > if (!static_command_line) > @@ -533,6 +539,22 @@ static void __init setup_command_line(char *command_line) > } > strcpy(saved_command_line + xlen, boot_command_line); > strcpy(static_command_line + xlen, command_line); > + > + if (ilen) { > + /* > + * Append supplemental init boot args to saved_command_line > + * so that user can check what command line options passed > + * to init. > + */ > + len = strlen(saved_command_line); > + if (!strstr(boot_command_line, " -- ")) { > + strcpy(saved_command_line + len, " -- "); > + len += 4; > + } else > + saved_command_line[len++] = ' '; > + > + strcpy(saved_command_line + len, extra_init_args); > + } This isn't safe because it will destroy any argument with " -- " in quotes and anything after it. For example, booting with: thing=on acpi_osi="! -- " other=setting will wreck acpi_osi's value and potentially overwrite "other=settings", etc. (Yes, this seems very unlikely, but you can't treat " -- " as special, the command line string must be correct parsed for double quotes, as parse_args() does.) > } > > /* > @@ -759,6 +781,9 @@ asmlinkage __visible void __init start_kernel(void) > if (!IS_ERR_OR_NULL(after_dashes)) > parse_args("Setting init args", after_dashes, NULL, 0, -1, -1, > NULL, set_init_arg); > + if (extra_init_args) > + parse_args("Setting extra init args", extra_init_args, > + NULL, 0, -1, -1, NULL, set_init_arg); Here is where you can append the extra_init_args, since parse_args() will have done the work to find after_dashes correctly. -Kees > > /* > * These use large bootmem allocations and must precede >
On Fri, Feb 07, 2020 at 10:03:16AM -0800, Kees Cook wrote: > > + > > + if (ilen) { > > + /* > > + * Append supplemental init boot args to saved_command_line > > + * so that user can check what command line options passed > > + * to init. > > + */ > > + len = strlen(saved_command_line); > > + if (!strstr(boot_command_line, " -- ")) { > > + strcpy(saved_command_line + len, " -- "); > > + len += 4; > > + } else > > + saved_command_line[len++] = ' '; > > + > > + strcpy(saved_command_line + len, extra_init_args); > > + } > > This isn't safe because it will destroy any argument with " -- " in > quotes and anything after it. For example, booting with: > > thing=on acpi_osi="! -- " other=setting > > will wreck acpi_osi's value and potentially overwrite "other=settings", > etc. > > (Yes, this seems very unlikely, but you can't treat " -- " as special, > the command line string must be correct parsed for double quotes, as > parse_args() does.) > I think it won't overwrite anything, it will just leave out the " -- " that should have been added? I wonder if this is necessary, though -- since commit b88c50ac304a ("log arguments and environment passed to init") the init arguments will be in the kernel log anyway.
On Fri, 7 Feb 2020 10:03:16 -0800 Kees Cook <keescook@chromium.org> wrote: > > static void __init setup_command_line(char *command_line) > > { > > - size_t len, xlen = 0; > > + size_t len, xlen = 0, ilen = 0; > > > > if (extra_command_line) > > xlen = strlen(extra_command_line); > > + if (extra_init_args) > > + ilen = strlen(extra_init_args) + 4; /* for " -- " */ > > > > len = xlen + strlen(boot_command_line) + 1; > > > > - saved_command_line = memblock_alloc(len, SMP_CACHE_BYTES); > > + saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES); > > if (!saved_command_line) > > - panic("%s: Failed to allocate %zu bytes\n", __func__, len); > > + panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen); > > > > static_command_line = memblock_alloc(len, SMP_CACHE_BYTES); > > if (!static_command_line) > > @@ -533,6 +539,22 @@ static void __init setup_command_line(char *command_line) > > } > > strcpy(saved_command_line + xlen, boot_command_line); > > strcpy(static_command_line + xlen, command_line); > > + > > + if (ilen) { > > + /* > > + * Append supplemental init boot args to saved_command_line > > + * so that user can check what command line options passed > > + * to init. > > + */ > > + len = strlen(saved_command_line); > > + if (!strstr(boot_command_line, " -- ")) { > > + strcpy(saved_command_line + len, " -- "); > > + len += 4; > > + } else > > + saved_command_line[len++] = ' '; > > + > > + strcpy(saved_command_line + len, extra_init_args); > > + } > > This isn't safe because it will destroy any argument with " -- " in > quotes and anything after it. For example, booting with: > > thing=on acpi_osi="! -- " other=setting > > will wreck acpi_osi's value and potentially overwrite "other=settings", > etc. > > (Yes, this seems very unlikely, but you can't treat " -- " as special, > the command line string must be correct parsed for double quotes, as > parse_args() does.) > This is not the args you are looking for. ;-) There is a slight bug, but not as bad as you may think it is. bootconfig (when added to the command line) will look for a json like file appended to the initrd, and it will parse that. That's what all the xbc_*() functions do (extended boot commandline). If one of the options in that json like file is "init", then it will create the extra_init_args, which will make ilen greater than zero. The above if statement looks for that ' -- ', and if it doesn't find it (strcmp() returns NULL when not found) it will than append " -- " to the boot_command_line. If it is found, then the " -- " is not added. In either case, the init args found in the json like file in the initrd is appended to the saved_command_line. I did say there's a slight bug here. If you have your condition, and you add init arguments to that json file, it wont properly add the " -- ", and the init arguments in that file will be ignored. That should be fixed, and I think I was able to do that below. I also noticed that we don't properly look for "bootconfig" either. -- Steve > > } > > > > /* > > @@ -759,6 +781,9 @@ asmlinkage __visible void __init start_kernel(void) > > if (!IS_ERR_OR_NULL(after_dashes)) > > parse_args("Setting init args", after_dashes, NULL, 0, -1, -1, > > NULL, set_init_arg); > > + if (extra_init_args) > > + parse_args("Setting extra init args", extra_init_args, > > + NULL, 0, -1, -1, NULL, set_init_arg); diff --git a/init/main.c b/init/main.c index 491f1cdb3105..113c8244e5f0 100644 --- a/init/main.c +++ b/init/main.c @@ -142,6 +142,15 @@ static char *extra_command_line; /* Extra init arguments */ static char *extra_init_args; +#ifdef CONFIG_BOOT_CONFIG +/* Is bootconfig on command line? */ +static bool bootconfig_found; +static bool initargs_found; +#else +# define bootconfig_found false +# define initargs_found false +#endif + static char *execute_command; static char *ramdisk_execute_command; @@ -336,17 +345,32 @@ u32 boot_config_checksum(unsigned char *p, u32 size) return ret; } +static int __init bootconfig_params(char *param, char *val, + const char *unused, void *arg) +{ + if (strcmp(param, "bootconfig") == 0) { + bootconfig_found = true; + } else if (strcmp(param, "--") == 0) { + initargs_found = true; + } + return 0; +} + static void __init setup_boot_config(const char *cmdline) { + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; u32 size, csum; char *data, *copy; const char *p; u32 *hdr; int ret; - p = strstr(cmdline, "bootconfig"); - if (!p || (p != cmdline && !isspace(*(p-1))) || - (p[10] && !isspace(p[10]))) + /* All fall through to do_early_param. */ + strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE); + parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL, + bootconfig_params); + + if (!bootconfig_found) return; if (!initrd_end) @@ -563,11 +587,12 @@ static void __init setup_command_line(char *command_line) * to init. */ len = strlen(saved_command_line); - if (!strstr(boot_command_line, " -- ")) { + if (initargs_found) { + saved_command_line[len++] = ' '; + } else { strcpy(saved_command_line + len, " -- "); len += 4; - } else - saved_command_line[len++] = ' '; + } strcpy(saved_command_line + len, extra_init_args); }
On Fri, Feb 07, 2020 at 02:46:03PM -0500, Steven Rostedt wrote: > On Fri, 7 Feb 2020 10:03:16 -0800 > Kees Cook <keescook@chromium.org> wrote: > > > + len = strlen(saved_command_line); > > > + if (!strstr(boot_command_line, " -- ")) { > > > + strcpy(saved_command_line + len, " -- "); > > > + len += 4; > > > + } else > > > + saved_command_line[len++] = ' '; > > > + > > > + strcpy(saved_command_line + len, extra_init_args); > > > + } > > > > This isn't safe because it will destroy any argument with " -- " in > > quotes and anything after it. For example, booting with: > > > > thing=on acpi_osi="! -- " other=setting > > > > will wreck acpi_osi's value and potentially overwrite "other=settings", > > etc. > > > > (Yes, this seems very unlikely, but you can't treat " -- " as special, > > the command line string must be correct parsed for double quotes, as > > parse_args() does.) > > > > This is not the args you are looking for. ;-) > > There is a slight bug, but not as bad as you may think it is. > bootconfig (when added to the command line) will look for a json like > file appended to the initrd, and it will parse that. That's what all the > xbc_*() functions do (extended boot commandline). If one of the options > in that json like file is "init", then it will create the > extra_init_args, which will make ilen greater than zero. > > The above if statement looks for that ' -- ', and if it doesn't find it > (strcmp() returns NULL when not found) it will than append " -- " to > the boot_command_line. If it is found, then the " -- " is not added. In > either case, the init args found in the json like file in the initrd is > appended to the saved_command_line. > > I did say there's a slight bug here. If you have your condition, and > you add init arguments to that json file, it wont properly add the " -- > ", and the init arguments in that file will be ignored. Ah, right, it's even more slight, sorry, I had the strstr() in my head still. So, yes, with an "init" section and a very goofy " -- " present in a kernel bootparam string value, the appended init args will be parsed as kernel options.
On Fri, Feb 07, 2020 at 02:46:03PM -0500, Steven Rostedt wrote: > > diff --git a/init/main.c b/init/main.c > index 491f1cdb3105..113c8244e5f0 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -142,6 +142,15 @@ static char *extra_command_line; > /* Extra init arguments */ > static char *extra_init_args; > > +#ifdef CONFIG_BOOT_CONFIG > +/* Is bootconfig on command line? */ > +static bool bootconfig_found; > +static bool initargs_found; > +#else > +# define bootconfig_found false > +# define initargs_found false > +#endif > + > static char *execute_command; > static char *ramdisk_execute_command; > > @@ -336,17 +345,32 @@ u32 boot_config_checksum(unsigned char *p, u32 size) > return ret; > } > > +static int __init bootconfig_params(char *param, char *val, > + const char *unused, void *arg) > +{ > + if (strcmp(param, "bootconfig") == 0) { > + bootconfig_found = true; > + } else if (strcmp(param, "--") == 0) { > + initargs_found = true; > + } > + return 0; > +} > + I came across this as I was poking around some of the command line parsing. AFAICT, initargs_found will never be set to true here, because parse_args handles "--" itself by immediately returning: it doesn't invoke the callback for it. So you'd instead have to check the return of parse_args("bootconfig"...) to detect the initargs_found case. > static void __init setup_boot_config(const char *cmdline) > { > + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; > u32 size, csum; > char *data, *copy; > const char *p; > u32 *hdr; > int ret; > > - p = strstr(cmdline, "bootconfig"); > - if (!p || (p != cmdline && !isspace(*(p-1))) || > - (p[10] && !isspace(p[10]))) > + /* All fall through to do_early_param. */ > + strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE); > + parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL, > + bootconfig_params); > + > + if (!bootconfig_found) > return;
On Sat, 1 Aug 2020 22:33:18 -0400 Arvind Sankar <nivedita@alum.mit.edu> wrote: > On Fri, Feb 07, 2020 at 02:46:03PM -0500, Steven Rostedt wrote: > > > > diff --git a/init/main.c b/init/main.c > > index 491f1cdb3105..113c8244e5f0 100644 > > --- a/init/main.c > > +++ b/init/main.c > > @@ -142,6 +142,15 @@ static char *extra_command_line; > > /* Extra init arguments */ > > static char *extra_init_args; > > > > +#ifdef CONFIG_BOOT_CONFIG > > +/* Is bootconfig on command line? */ > > +static bool bootconfig_found; > > +static bool initargs_found; > > +#else > > +# define bootconfig_found false > > +# define initargs_found false > > +#endif > > + > > static char *execute_command; > > static char *ramdisk_execute_command; > > > > @@ -336,17 +345,32 @@ u32 boot_config_checksum(unsigned char *p, u32 size) > > return ret; > > } > > > > +static int __init bootconfig_params(char *param, char *val, > > + const char *unused, void *arg) > > +{ > > + if (strcmp(param, "bootconfig") == 0) { > > + bootconfig_found = true; > > + } else if (strcmp(param, "--") == 0) { > > + initargs_found = true; > > + } > > + return 0; > > +} > > + > > I came across this as I was poking around some of the command line > parsing. AFAICT, initargs_found will never be set to true here, because > parse_args handles "--" itself by immediately returning: it doesn't > invoke the callback for it. So you'd instead have to check the return of > parse_args("bootconfig"...) to detect the initargs_found case. Oops, good catch! Does this fixes the problem? From b078e8b02ad54aea74f8c3645fc11dd3a1cdc1e7 Mon Sep 17 00:00:00 2001 From: Masami Hiramatsu <mhiramat@kernel.org> Date: Mon, 3 Aug 2020 23:57:29 +0900 Subject: [PATCH] bootconfig: Fix to find the initargs correctly Since the parse_args() stops parsing at '--', bootconfig_params() will never get the '--' as param and initargs_found never be true. In the result, if we pass some init arguments via the bootconfig, those are always appended to the kernel command line with '--' and user will see double '--'. To fix this correctly, check the return value of parse_args() and set initargs_found true if the return value is not an error but a valid address. Fixes: f61872bb58a1 ("bootconfig: Use parse_args() to find bootconfig and '--'") Cc: stable@vger.kernel.org Reported-by: Arvind Sankar <nivedita@alum.mit.edu> Suggested-by: Arvind Sankar <nivedita@alum.mit.edu> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- init/main.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/init/main.c b/init/main.c index 0ead83e86b5a..627f9230dbe8 100644 --- a/init/main.c +++ b/init/main.c @@ -387,8 +387,6 @@ static int __init bootconfig_params(char *param, char *val, { if (strcmp(param, "bootconfig") == 0) { bootconfig_found = true; - } else if (strcmp(param, "--") == 0) { - initargs_found = true; } return 0; } @@ -399,19 +397,23 @@ static void __init setup_boot_config(const char *cmdline) const char *msg; int pos; u32 size, csum; - char *data, *copy; + char *data, *copy, *err; int ret; /* Cut out the bootconfig data even if we have no bootconfig option */ data = get_boot_config_from_initrd(&size, &csum); strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE); - parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL, - bootconfig_params); + err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL, + bootconfig_params); - if (!bootconfig_found) + if (IS_ERR(err) || !bootconfig_found) return; + /* parse_args() stops at '--' and returns an address */ + if (!IS_ERR(err) && err) + initargs_found = true; + if (!data) { pr_err("'bootconfig' found on command line, but no bootconfig found\n"); return;
On Tue, Aug 04, 2020 at 12:03:45AM +0900, Masami Hiramatsu wrote: > On Sat, 1 Aug 2020 22:33:18 -0400 > Arvind Sankar <nivedita@alum.mit.edu> wrote: > > > > I came across this as I was poking around some of the command line > > parsing. AFAICT, initargs_found will never be set to true here, because > > parse_args handles "--" itself by immediately returning: it doesn't > > invoke the callback for it. So you'd instead have to check the return of > > parse_args("bootconfig"...) to detect the initargs_found case. > > Oops, good catch! > Does this fixes the problem? Note I found the issue by code inspection, I don't have an actual test case. But the change looks good to me, with one comment below. > > strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE); > - parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL, > - bootconfig_params); > + err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL, > + bootconfig_params); > > - if (!bootconfig_found) > + if (IS_ERR(err) || !bootconfig_found) > return; > > + /* parse_args() stops at '--' and returns an address */ > + if (!IS_ERR(err) && err) > + initargs_found = true; > + I think you can drop the second IS_ERR, since we already checked that. > if (!data) { > pr_err("'bootconfig' found on command line, but no bootconfig found\n"); > return; > -- > 2.25.1
On Mon, 3 Aug 2020 11:29:59 -0400 Arvind Sankar <nivedita@alum.mit.edu> wrote: > > + /* parse_args() stops at '--' and returns an address */ > > + if (!IS_ERR(err) && err) > > + initargs_found = true; > > + > > I think you can drop the second IS_ERR, since we already checked that. Masami, Can you send this with the update as a normal patch (not a Cc to this thread). That way it gets caught by my patchwork scanning of my inbox. Thanks! (/me is currently going through all his patchwork patches to pull in for the merge window.) -- Steve
On Mon, 3 Aug 2020 13:22:38 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 3 Aug 2020 11:29:59 -0400 > Arvind Sankar <nivedita@alum.mit.edu> wrote: > > > > + /* parse_args() stops at '--' and returns an address */ > > > + if (!IS_ERR(err) && err) > > > + initargs_found = true; > > > + > > > > I think you can drop the second IS_ERR, since we already checked that. > > Masami, > > Can you send this with the update as a normal patch (not a Cc to this > thread). That way it gets caught by my patchwork scanning of my inbox. OK, I'll update it. > > Thanks! > > (/me is currently going through all his patchwork patches to pull in > for the merge window.) Thank you! > > -- Steve
diff --git a/init/main.c b/init/main.c index c0017d9d16e7..dd7da62d99a5 100644 --- a/init/main.c +++ b/init/main.c @@ -139,6 +139,8 @@ char *saved_command_line; static char *static_command_line; /* Untouched extra command line */ static char *extra_command_line; +/* Extra init arguments */ +static char *extra_init_args; static char *execute_command; static char *ramdisk_execute_command; @@ -372,6 +374,8 @@ static void __init setup_boot_config(void) pr_info("Load boot config: %d bytes\n", size); /* keys starting with "kernel." are passed via cmdline */ extra_command_line = xbc_make_cmdline("kernel"); + /* Also, "init." keys are init arguments */ + extra_init_args = xbc_make_cmdline("init"); } } #else @@ -507,16 +511,18 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { } */ static void __init setup_command_line(char *command_line) { - size_t len, xlen = 0; + size_t len, xlen = 0, ilen = 0; if (extra_command_line) xlen = strlen(extra_command_line); + if (extra_init_args) + ilen = strlen(extra_init_args) + 4; /* for " -- " */ len = xlen + strlen(boot_command_line) + 1; - saved_command_line = memblock_alloc(len, SMP_CACHE_BYTES); + saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES); if (!saved_command_line) - panic("%s: Failed to allocate %zu bytes\n", __func__, len); + panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen); static_command_line = memblock_alloc(len, SMP_CACHE_BYTES); if (!static_command_line) @@ -533,6 +539,22 @@ static void __init setup_command_line(char *command_line) } strcpy(saved_command_line + xlen, boot_command_line); strcpy(static_command_line + xlen, command_line); + + if (ilen) { + /* + * Append supplemental init boot args to saved_command_line + * so that user can check what command line options passed + * to init. + */ + len = strlen(saved_command_line); + if (!strstr(boot_command_line, " -- ")) { + strcpy(saved_command_line + len, " -- "); + len += 4; + } else + saved_command_line[len++] = ' '; + + strcpy(saved_command_line + len, extra_init_args); + } } /* @@ -759,6 +781,9 @@ asmlinkage __visible void __init start_kernel(void) if (!IS_ERR_OR_NULL(after_dashes)) parse_args("Setting init args", after_dashes, NULL, 0, -1, -1, NULL, set_init_arg); + if (extra_init_args) + parse_args("Setting extra init args", extra_init_args, + NULL, 0, -1, -1, NULL, set_init_arg); /* * These use large bootmem allocations and must precede
Since the current kernel command line is too short to describe long and many options for init (e.g. systemd command line options), this allows admin to use boot config for init command line. All init command line under "init." keywords will be passed to init. For example, init.systemd { unified_cgroup_hierarchy = 1 debug_shell default_timeout_start_sec = 60 } Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- init/main.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)