* Re: [PATCH] bootconfig: Use parse_args() to find bootconfig and '--'
2020-02-08 0:26 [PATCH] bootconfig: Use parse_args() to find bootconfig and '--' Steven Rostedt
@ 2020-02-08 0:49 ` Steven Rostedt
2020-02-08 0:56 ` Kees Cook
2020-02-08 1:41 ` Masami Hiramatsu
2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2020-02-08 0:49 UTC (permalink / raw)
To: LKML
Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Frank Rowand,
Randy Dunlap, Namhyung Kim, Tim Bird, Jiri Olsa,
Arnaldo Carvalho de Melo, Tom Zanussi, Rob Herring, Andrew Morton,
Thomas Gleixner, Greg Kroah-Hartman, Alexey Dobriyan,
Jonathan Corbet, Linus Torvalds, linux-doc, linux-fsdevel
On Fri, 7 Feb 2020 19:26:32 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> 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;
I have a v2 of this patch that removes the variable "p" as I now get an
unused variable because of it.
-- Steve
> u32 *hdr;
> int ret;
>
> - p = strstr(cmdline, "bootconfig");
> - if (!p || (p != cmdline && !isspace(*(p-1))) ||
> - (p[10] && !isspace(p[10])))
> + 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;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] bootconfig: Use parse_args() to find bootconfig and '--'
2020-02-08 0:26 [PATCH] bootconfig: Use parse_args() to find bootconfig and '--' Steven Rostedt
2020-02-08 0:49 ` Steven Rostedt
@ 2020-02-08 0:56 ` Kees Cook
2020-02-08 1:41 ` Masami Hiramatsu
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2020-02-08 0:56 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Masami Hiramatsu, Ingo Molnar, Frank Rowand, Randy Dunlap,
Namhyung Kim, Tim Bird, Jiri Olsa, Arnaldo Carvalho de Melo,
Tom Zanussi, Rob Herring, Andrew Morton, Thomas Gleixner,
Greg Kroah-Hartman, Alexey Dobriyan, Jonathan Corbet,
Linus Torvalds, linux-doc, linux-fsdevel
On Fri, Feb 07, 2020 at 07:26:32PM -0500, Steven Rostedt wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> The current implementation does a naive search of "bootconfig" on the kernel
> command line. But this could find "bootconfig" that is part of another
> option in quotes (although highly unlikely). But it also needs to find '--'
> on the kernel command line to know if it should append a '--' or not when a
> bootconfig in the initrd file has an "init" section. The check uses the
> naive strstr() to find to see if it exists. But this can return a false
> positive if it exists in an option and then the "init" section in the initrd
> will not be appended properly.
>
> Using parse_args() to find both of these will solve both of these problems.
>
> Link: https://lore.kernel.org/r/202002070954.C18E7F58B@keescook
>
> Fixes: 7495e0926fdf3 ("bootconfig: Only load bootconfig if "bootconfig" is on the kernel cmdline")
> Fixes: 1319916209ce8 ("bootconfig: init: Allow admin to use bootconfig for init command line")
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Cool; thanks for fixing this!
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> init/main.c | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index 491f1cdb3105..e7261f1a3523 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,31 @@ 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])))
> + 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 +586,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);
> }
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] bootconfig: Use parse_args() to find bootconfig and '--'
2020-02-08 0:26 [PATCH] bootconfig: Use parse_args() to find bootconfig and '--' Steven Rostedt
2020-02-08 0:49 ` Steven Rostedt
2020-02-08 0:56 ` Kees Cook
@ 2020-02-08 1:41 ` Masami Hiramatsu
2 siblings, 0 replies; 4+ messages in thread
From: Masami Hiramatsu @ 2020-02-08 1:41 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Masami Hiramatsu, Kees Cook, Ingo Molnar, Frank Rowand,
Randy Dunlap, Namhyung Kim, Tim Bird, Jiri Olsa,
Arnaldo Carvalho de Melo, Tom Zanussi, Rob Herring, Andrew Morton,
Thomas Gleixner, Greg Kroah-Hartman, Alexey Dobriyan,
Jonathan Corbet, Linus Torvalds, linux-doc, linux-fsdevel
On Fri, 7 Feb 2020 19:26:32 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> The current implementation does a naive search of "bootconfig" on the kernel
> command line. But this could find "bootconfig" that is part of another
> option in quotes (although highly unlikely). But it also needs to find '--'
> on the kernel command line to know if it should append a '--' or not when a
> bootconfig in the initrd file has an "init" section. The check uses the
> naive strstr() to find to see if it exists. But this can return a false
> positive if it exists in an option and then the "init" section in the initrd
> will not be appended properly.
>
> Using parse_args() to find both of these will solve both of these problems.
Thanks Steve and Kees! This looks good to me.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you,
>
> Link: https://lore.kernel.org/r/202002070954.C18E7F58B@keescook
>
> Fixes: 7495e0926fdf3 ("bootconfig: Only load bootconfig if "bootconfig" is on the kernel cmdline")
> Fixes: 1319916209ce8 ("bootconfig: init: Allow admin to use bootconfig for init command line")
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> init/main.c | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index 491f1cdb3105..e7261f1a3523 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,31 @@ 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])))
> + 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 +586,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);
> }
> --
> 2.20.1
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread