[PATCH 1/2] launch: Rationalize how we construct the Linux kernel command line.
This is just code refactoring. --- src/launch.c | 172 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 109 insertions(+), 63 deletions(-) diff --git a/src/launch.c b/src/launch.c index 46d7ab9..84d5e82 100644 --- a/src/launch.c +++ b/src/launch.c @@ -293,9 +293,7 @@ guestfs_impl_config (guestfs_h *g, #endif #if defined(__aarch64__) -#define EARLYPRINTK " earlyprintk=pl011,0x9000000" -#else -#define EARLYPRINTK "" +#define EARLYPRINTK "earlyprintk=pl011,0x9000000" #endif /** @@ -327,75 +325,123 @@ char * guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, int flags) { - char root[64] = ""; + CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (argv); char *term = getenv ("TERM"); - char *ret; bool tcg = flags & APPLIANCE_COMMAND_LINE_IS_TCG; - char lpj_s[64] = ""; - if (appliance_dev) - snprintf (root, sizeof root, " root=%s", appliance_dev); + /* We assemble the kernel command line by simply joining the final + * list of strings with spaces. This means (a) the strings are not + * quoted (it's not clear if the kernel can handle quoting in any + * case), and (b) we can append multiple parameters in a single + * argument, as we must do for the g->append parameter. + */ + + /* Force kernel to panic if daemon exits. */ + guestfs_int_add_string (g, &argv, "panic=1"); + +#ifdef __arm__ + guestfs_int_add_sprintf (g, &argv, " mem=%dM", g->memsize); +#endif + +#ifdef __i386__ + /* Workaround for RHBZ#857026. */ + guestfs_int_add_string (g, &argv, "noapic"); +#endif + + /* Serial console. */ + guestfs_int_add_string (g, &argv, SERIAL_CONSOLE); + +#ifdef EARLYPRINTK + /* Get messages from early boot. */ + guestfs_int_add_string (g, &argv, EARLYPRINTK); +#endif + +#ifdef __aarch64__ + guestfs_int_add_string (g, &argv, "ignore_loglevel"); + + /* This option turns off the EFI RTC device. QEMU VMs don't + * currently provide EFI, and if the device is compiled in it + * will try to call the EFI function GetTime unconditionally + * (causing a call to NULL). However this option requires a + * non-upstream patch. + */ + guestfs_int_add_string (g, &argv, "efi-rtc=noprobe"); +#endif + + /* RHBZ#1404287 */ + guestfs_int_add_string (g, &argv, "edd=off"); + + /* For slow systems (RHBZ#480319, RHBZ#1096579). */ + guestfs_int_add_string (g, &argv, "udevtimeout=6000"); + + /* Same as above, for newer udevd. */ + guestfs_int_add_string (g, &argv, "udev.event-timeout=6000"); + + /* Fix for RHBZ#502058. */ + guestfs_int_add_string (g, &argv, "no_timer_check"); if (tcg) { const int lpj = guestfs_int_get_lpj (g); if (lpj > 0) - snprintf (lpj_s, sizeof lpj_s, " lpj=%d", lpj); + guestfs_int_add_sprintf (g, &argv, "lpj=%d", lpj); } - ret = safe_asprintf - (g, - "panic=1" /* force kernel to panic if daemon exits */ -#ifdef __arm__ - " mem=%dM" -#endif -#ifdef __i386__ - " noapic" /* workaround for RHBZ#857026 */ -#endif - " " SERIAL_CONSOLE /* serial console */ - EARLYPRINTK /* get messages from early boot */ -#ifdef __aarch64__ - " ignore_loglevel" - /* This option turns off the EFI RTC device. QEMU VMs don't - * currently provide EFI, and if the device is compiled in it - * will try to call the EFI function GetTime unconditionally - * (causing a call to NULL). However this option requires a - * non-upstream patch. - */ - " efi-rtc=noprobe" -#endif - " edd=off" /* RHBZ#1404287 */ - " udevtimeout=6000"/* for slow systems (RHBZ#480319, RHBZ#1096579) */ - " udev.event-timeout=6000" /* for newer udevd */ - " no_timer_check" /* fix for RHBZ#502058 */ - "%s" /* lpj */ - " printk.time=1" /* display timestamp before kernel messages */ - " cgroup_disable=memory" /* saves us about 5 MB of RAM */ - " usbcore.nousb" /* disable USB, only saves about 1ms */ - " cryptomgr.notests" /* disable crypto tests, saves 28ms */ - " tsc=reliable" /* don't synch TSCs when using SMP, - saves 21ms for each secondary vCPU */ - " 8250.nr_uarts=1" /* don't scan all 8250 UARTS */ - "%s" /* root=appliance_dev */ - " %s" /* selinux */ - " %s" /* quiet/verbose */ - "%s" /* network */ - " TERM=%s" /* TERM environment variable */ - "%s%s" /* handle identifier */ - "%s%s", /* append */ -#ifdef __arm__ - g->memsize, -#endif - lpj_s, - root, - g->selinux ? "selinux=1 enforcing=0" : "selinux=0", - g->verbose ? "guestfs_verbose=1" : "quiet", - g->enable_network ? " guestfs_network=1" : "", - term ? term : "linux", - STRNEQ (g->identifier, "") ? " guestfs_identifier=" : "", - g->identifier, - g->append ? " " : "", g->append ? g->append : ""); - - return ret; + /* Display timestamp before kernel messages. */ + guestfs_int_add_string (g, &argv, "printk.time=1"); + + /* Saves us about 5 MB of RAM. */ + guestfs_int_add_string (g, &argv, "cgroup_disable=memory"); + + /* Disable USB, only saves about 1ms. */ + guestfs_int_add_string (g, &argv, "usbcore.nousb"); + + /* Disable crypto tests, saves 28ms. */ + guestfs_int_add_string (g, &argv, "cryptomgr.notests"); + + /* Don't synch TSCs when using SMP. Saves 21ms for each secondary vCPU. */ + guestfs_int_add_string (g, &argv, "tsc=reliable"); + + /* Don't scan all 8250 UARTS. */ + guestfs_int_add_string (g, &argv, "8250.nr_uarts=1"); + + /* Tell supermin about the appliance device. */ + if (appliance_dev) + guestfs_int_add_sprintf (g, &argv, "root=%s", appliance_dev); + + /* SELinux - deprecated setting, never worked and should not be enabled. */ + if (g->selinux) + guestfs_int_add_string (g, &argv, "selinux=1 enforcing=0"); + else + guestfs_int_add_string (g, &argv, "selinux=0"); + + /* Quiet/verbose. */ + if (g->verbose) + guestfs_int_add_string (g, &argv, "guestfs_verbose=1"); + else + guestfs_int_add_string (g, &argv, "quiet"); + + /* Network. */ + if (g->enable_network) + guestfs_int_add_string (g, &argv, "guestfs_network=1"); + + /* TERM environment variable. */ + if (term) + guestfs_int_add_sprintf (g, &argv, "TERM=%s", term); + else + guestfs_int_add_string (g, &argv, "TERM=linux"); + + /* Handle identifier. */ + if (STRNEQ (g->identifier, "")) + guestfs_int_add_sprintf (g, &argv, "guestfs_identifier=%s", g->identifier); + + /* Append extra arguments. */ + if (g->append) + guestfs_int_add_string (g, &argv, g->append); + + guestfs_int_end_stringsbuf (g, &argv); + + /* Caller frees. */ + return guestfs_int_join_strings (" ", argv.argv); } /** -- 2.10.2
Make sure it is reasonable before we pass it through to the kernel command line. I don't believe this is exploitable, but it might cause obscure bugs. --- src/launch.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/launch.c b/src/launch.c index 84d5e82..ee2a23d 100644 --- a/src/launch.c +++ b/src/launch.c @@ -39,6 +39,8 @@ #include <assert.h> #include <libintl.h> +#include "c-ctype.h" + #include "guestfs.h" #include "guestfs-internal.h" #include "guestfs-internal-actions.h" @@ -284,6 +286,28 @@ guestfs_impl_config (guestfs_h *g, return 0; } +/** + * Check that the $TERM environment variable is reasonable before + * we pass it through to the appliance. + */ +static int +valid_term (const char *term) +{ + size_t len = strlen (term); + + if (len == 0 || len > 16) + return 0; + + while (len > 0) { + char c = *term++; + len--; + if (!c_isalnum (c) && c != '-' && c != '_') + return 0; + } + + return 1; +} + #if defined(__powerpc64__) #define SERIAL_CONSOLE "console=hvc0 console=ttyS0" #elif defined(__arm__) || defined(__aarch64__) @@ -425,7 +449,7 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, guestfs_int_add_string (g, &argv, "guestfs_network=1"); /* TERM environment variable. */ - if (term) + if (term && valid_term (term)) guestfs_int_add_sprintf (g, &argv, "TERM=%s", term); else guestfs_int_add_string (g, &argv, "TERM=linux"); -- 2.10.2
On Sunday, 18 December 2016 20:09:29 CET Richard W.M. Jones wrote:
Make sure it is reasonable before we pass it through to the kernel command line. I don't believe this is exploitable, but it might cause obscure bugs. --- src/launch.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/src/launch.c b/src/launch.c index 84d5e82..ee2a23d 100644 --- a/src/launch.c +++ b/src/launch.c @@ -39,6 +39,8 @@ #include <assert.h> #include <libintl.h>
+#include "c-ctype.h" + #include "guestfs.h" #include "guestfs-internal.h" #include "guestfs-internal-actions.h" @@ -284,6 +286,28 @@ guestfs_impl_config (guestfs_h *g, return 0; }
+/** + * Check that the $TERM environment variable is reasonable before + * we pass it through to the appliance. + */ +static int +valid_term (const char *term)
I guess the return value can be bool.
+{ + size_t len = strlen (term); + + if (len == 0 || len > 16) + return 0; + + while (len > 0) { + char c = *term++; + len--; + if (!c_isalnum (c) && c != '-' && c != '_') + return 0; + }
The loop is fine already, maybe the need to use len can be dropped: for (; *term; ++term) { char c = *term; if (!c_isalnum (c) && c != '-' && c != '_') return 0; } Thanks, -- Pino Toscano
Thanks - pushed with all but one of the updates suggested. I didn't rewrite the loop to not use len, as that code was copied from the already working code in src/drives.c. Possibly we need some kind of "validate string only contains chars in this set" function. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Sunday, 18 December 2016 20:09:28 CET Richard W.M. Jones wrote:
This is just code refactoring. ---
Mostly LGTM, just one note.
+ return guestfs_int_join_strings (" ", argv.argv);
Currently, safe_asprintf aborts on ENOMEM errors, while guestfs_int_join_strings just returns NULL on that situation -- most probably the abort callback needs to be invoked manually: char *ret; ... ret = guestfs_int_join_strings (" ", argv.argv); if (ret == NULL) g->abort_cb (); return ret; Thanks, -- Pino Toscano
participants (2)
-
Pino Toscano -
Richard W.M. Jones