| Message ID | 20210212094524.170861-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] libkmod-config: revamp kcmdline parsing into a state machine | expand |
+++ Lucas De Marchi [12/02/21 01:45 -0800]: >It was reported that grub mangles the kernel cmdline. It turns > > acpi_cpufreq.dyndbg="file drivers/cpufreq/acpi-cpufreq.c +mpf" > > into > > "acpi_cpufreq.dyndbg=file drivers/cpufreq/acpi-cpufreq.c +mpf" > >However, even though we could blame grub for doing that, the kernel >happily accepts and re-quotes it when the module is built-in. >So, it's better if kmod also understands it this way and does the same. > >Here we basically add additional code to un-mangle it, moving the quote >in way that is acceptable to pass through init_module(). Note that the >interface [f]init_module() gives us mandates the quote to be part of the >value: the module name is not passed and the options are separated by >space. > >Reported-by: Jiri Slaby <jirislaby@kernel.org> >Link: https://bugzilla.suse.com/show_bug.cgi?id=1181111#c10 Hi Lucas, Thanks a lot for working on this. I applied this patchset on top of kmod master and after some light testing it appears to be able to handle the mangled quoting from grub now: Tested-by: Jessica Yu <jeyu@kernel.org>
On Mon, Feb 15, 2021 at 7:33 AM Jessica Yu <jeyu@kernel.org> wrote: > > +++ Lucas De Marchi [12/02/21 01:45 -0800]: > >It was reported that grub mangles the kernel cmdline. It turns > > > > acpi_cpufreq.dyndbg="file drivers/cpufreq/acpi-cpufreq.c +mpf" > > > > into > > > > "acpi_cpufreq.dyndbg=file drivers/cpufreq/acpi-cpufreq.c +mpf" > > > >However, even though we could blame grub for doing that, the kernel > >happily accepts and re-quotes it when the module is built-in. > >So, it's better if kmod also understands it this way and does the same. > > > >Here we basically add additional code to un-mangle it, moving the quote > >in way that is acceptable to pass through init_module(). Note that the > >interface [f]init_module() gives us mandates the quote to be part of the > >value: the module name is not passed and the options are separated by > >space. > > > >Reported-by: Jiri Slaby <jirislaby@kernel.org> > >Link: https://bugzilla.suse.com/show_bug.cgi?id=1181111#c10 > > Hi Lucas, > > Thanks a lot for working on this. I applied this patchset on top of > kmod master and after some light testing it appears to be able to > handle the mangled quoting from grub now: > > Tested-by: Jessica Yu <jeyu@kernel.org> thanks! Applied Lucas De Marchi
diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c index d3cd10d..2873f06 100644 --- a/libkmod/libkmod-config.c +++ b/libkmod/libkmod-config.c @@ -498,7 +498,7 @@ static int kmod_config_parse_kcmdline(struct kmod_config *config) { char buf[KCMD_LINE_SIZE]; int fd, err; - char *p, *modname, *param = NULL, *value = NULL; + char *p, *p_quote_start, *modname, *param = NULL, *value = NULL; bool is_quoted = false, iter = true; enum state { STATE_IGNORE, @@ -524,10 +524,23 @@ static int kmod_config_parse_kcmdline(struct kmod_config *config) } state = STATE_MODNAME; + p_quote_start = NULL; for (p = buf, modname = buf; iter; p++) { switch (*p) { case '"': is_quoted = !is_quoted; + + /* + * only allow starting quote as first char when looking + * for a modname: anything else is considered ill-formed + */ + if (is_quoted && state == STATE_MODNAME && p == modname) { + p_quote_start = p; + modname = p + 1; + } else if (state != STATE_VALUE) { + state = STATE_IGNORE; + } + break; case '\0': case '\n': @@ -550,6 +563,7 @@ static int kmod_config_parse_kcmdline(struct kmod_config *config) */ modname = p + 1; state = STATE_MODNAME; + p_quote_start = NULL; } break; case '.': @@ -576,10 +590,30 @@ static int kmod_config_parse_kcmdline(struct kmod_config *config) } if (state == STATE_COMPLETE) { + /* + * We may need to re-quote to unmangle what the + * bootloader passed. Example: grub passes the option as + * "parport.dyndbg=file drivers/parport/ieee1284_ops.c +mpf" + * instead of + * parport.dyndbg="file drivers/parport/ieee1284_ops.c +mpf" + */ + if (p_quote_start && p_quote_start < modname) { + /* + * p_quote_start + * | + * |modname param value + * || | | + * vv v v + * "parport\0dyndbg=file drivers/parport/ieee1284_ops.c +mpf" */ + memmove(p_quote_start, modname, value - modname); + value--; modname--; param--; + *value = '"'; + } kcmdline_parse_result(config, modname, param, value); /* start over on next iteration */ modname = p + 1; state = STATE_MODNAME; + p_quote_start = NULL; } } diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/correct.txt b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/correct.txt new file mode 100644 index 0000000..d80da6d --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/correct.txt @@ -0,0 +1,5 @@ +options psmouse foo +options parport dyndbg="file drivers/parport/ieee1284_ops.c +mpf" + +# End of configuration files. Dumping indexes now: + diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/module-param-kcmdline7/correct.txt b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/module-param-kcmdline7/correct.txt new file mode 100644 index 0000000..d80da6d --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/module-param-kcmdline7/correct.txt @@ -0,0 +1,5 @@ +options psmouse foo +options parport dyndbg="file drivers/parport/ieee1284_ops.c +mpf" + +# End of configuration files. Dumping indexes now: + diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/module-param-kcmdline7/proc/cmdline b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/module-param-kcmdline7/proc/cmdline new file mode 100644 index 0000000..86f9052 --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/module-param-kcmdline7/proc/cmdline @@ -0,0 +1 @@ +psmouse.foo parport.dyndbg="file drivers/parport/ieee1284_ops.c +mpf" quiet rw diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/proc/cmdline b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/proc/cmdline new file mode 100644 index 0000000..86f9052 --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline7/proc/cmdline @@ -0,0 +1 @@ +psmouse.foo parport.dyndbg="file drivers/parport/ieee1284_ops.c +mpf" quiet rw diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/correct.txt b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/correct.txt new file mode 100644 index 0000000..d80da6d --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/correct.txt @@ -0,0 +1,5 @@ +options psmouse foo +options parport dyndbg="file drivers/parport/ieee1284_ops.c +mpf" + +# End of configuration files. Dumping indexes now: + diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/module-param-kcmdline7/correct.txt b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/module-param-kcmdline7/correct.txt new file mode 100644 index 0000000..d80da6d --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/module-param-kcmdline7/correct.txt @@ -0,0 +1,5 @@ +options psmouse foo +options parport dyndbg="file drivers/parport/ieee1284_ops.c +mpf" + +# End of configuration files. Dumping indexes now: + diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/module-param-kcmdline7/proc/cmdline b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/module-param-kcmdline7/proc/cmdline new file mode 100644 index 0000000..86f9052 --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/module-param-kcmdline7/proc/cmdline @@ -0,0 +1 @@ +psmouse.foo parport.dyndbg="file drivers/parport/ieee1284_ops.c +mpf" quiet rw diff --git a/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/proc/cmdline b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/proc/cmdline new file mode 100644 index 0000000..eab04ad --- /dev/null +++ b/testsuite/rootfs-pristine/test-modprobe/module-param-kcmdline8/proc/cmdline @@ -0,0 +1 @@ +psmouse.foo "parport.dyndbg=file drivers/parport/ieee1284_ops.c +mpf" quiet rw diff --git a/testsuite/test-modprobe.c b/testsuite/test-modprobe.c index f6bed8b..dbc54f3 100644 --- a/testsuite/test-modprobe.c +++ b/testsuite/test-modprobe.c @@ -359,6 +359,56 @@ DEFINE_TEST(modprobe_param_kcmdline6, ); +static noreturn int modprobe_param_kcmdline7(const struct test *t) +{ + const char *progname = ABS_TOP_BUILDDIR "/tools/modprobe"; + const char *const args[] = { + progname, + "-c", + NULL, + }; + + test_spawn_prog(progname, args); + exit(EXIT_FAILURE); +} +DEFINE_TEST(modprobe_param_kcmdline7, + .description = "check if dots on other parts of kcmdline don't confuse our parser", + .config = { + [TC_UNAME_R] = "4.4.4", + [TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline7", + }, + .output = { + .out = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline7/correct.txt", + }, + .modules_loaded = "", + ); + + +static noreturn int modprobe_param_kcmdline8(const struct test *t) +{ + const char *progname = ABS_TOP_BUILDDIR "/tools/modprobe"; + const char *const args[] = { + progname, + "-c", + NULL, + }; + + test_spawn_prog(progname, args); + exit(EXIT_FAILURE); +} +DEFINE_TEST(modprobe_param_kcmdline8, + .description = "check if dots on other parts of kcmdline don't confuse our parser", + .config = { + [TC_UNAME_R] = "4.4.4", + [TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline8", + }, + .output = { + .out = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline8/correct.txt", + }, + .modules_loaded = "", + ); + + static noreturn int modprobe_force(const struct test *t) { const char *progname = ABS_TOP_BUILDDIR "/tools/modprobe";