From 327cce76ad2f435e84b7d53e64a1e0a5edbd568b Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 16 Feb 2018 22:04:35 +0100 Subject: [PATCH 1/5] conf: fix run_script_argv() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure that we allocate the buffer **after** we determined how much space we need in total. This fixes a SIGBUS/SIGSEGV Stéphane reported on aarch64 and armf. Reported-by: Stéphane Graber Signed-off-by: Christian Brauner --- src/lxc/conf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/lxc/conf.c b/src/lxc/conf.c index f75d69656..c208d5680 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -376,8 +376,6 @@ int run_script_argv(const char *name, unsigned int hook_version, if (size > INT_MAX) return -EFBIG; - buffer = alloca(size); - if (hook_version == 0) { size += strlen(hookname); size++; @@ -390,19 +388,19 @@ int run_script_argv(const char *name, unsigned int hook_version, if (size > INT_MAX) return -EFBIG; + } + buffer = alloca(size); + if (hook_version == 0) buf_pos = snprintf(buffer, size, "exec %s %s %s %s", script, name, section, hookname); - if (buf_pos < 0 || (size_t)buf_pos >= size) { - ERROR("Failed to create command line for script \"%s\"", script); - return -1; - } - } else { + else buf_pos = snprintf(buffer, size, "exec %s", script); - if (buf_pos < 0 || (size_t)buf_pos >= size) { - ERROR("Failed to create command line for script \"%s\"", script); - return -1; - } + if (buf_pos < 0 || (size_t)buf_pos >= size) { + ERROR("Failed to create command line for script \"%s\"", script); + return -1; + } + if (hook_version == 1) { ret = setenv("LXC_HOOK_TYPE", hookname, 1); if (ret < 0) { SYSERROR("Failed to set environment variable: " From 6f8d00d27964e548f66218408515993814bdd37f Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 16 Feb 2018 22:10:06 +0100 Subject: [PATCH 2/5] conf: use malloc() in run_script_argv() It seems dangerous to use alloca() as the arguments can be of indeterminate length and we might blow up the stack. Signed-off-by: Christian Brauner --- src/lxc/conf.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/lxc/conf.c b/src/lxc/conf.c index c208d5680..8995e0c16 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -358,6 +358,7 @@ int run_script_argv(const char *name, unsigned int hook_version, { int buf_pos, i, ret; char *buffer; + int fret = -1; size_t size = 0; if (hook_version == 0) @@ -390,14 +391,17 @@ int run_script_argv(const char *name, unsigned int hook_version, return -EFBIG; } - buffer = alloca(size); + buffer = malloc(size); + if (!buffer) + return -ENOMEM; + if (hook_version == 0) buf_pos = snprintf(buffer, size, "exec %s %s %s %s", script, name, section, hookname); else buf_pos = snprintf(buffer, size, "exec %s", script); if (buf_pos < 0 || (size_t)buf_pos >= size) { ERROR("Failed to create command line for script \"%s\"", script); - return -1; + goto on_error; } if (hook_version == 1) { @@ -405,7 +409,7 @@ int run_script_argv(const char *name, unsigned int hook_version, if (ret < 0) { SYSERROR("Failed to set environment variable: " "LXC_HOOK_TYPE=%s", hookname); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_HOOK_TYPE=%s", hookname); @@ -413,7 +417,7 @@ int run_script_argv(const char *name, unsigned int hook_version, if (ret < 0) { SYSERROR("Failed to set environment variable: " "LXC_HOOK_SECTION=%s", section); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_HOOK_SECTION=%s", section); @@ -421,13 +425,13 @@ int run_script_argv(const char *name, unsigned int hook_version, char *parent; if (!argsin[0]) - return -EINVAL; + goto on_error; ret = setenv("LXC_NET_TYPE", argsin[0], 1); if (ret < 0) { SYSERROR("Failed to set environment variable: " "LXC_NET_TYPE=%s", argsin[0]); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_NET_TYPE=%s", argsin[0]); @@ -438,7 +442,7 @@ int run_script_argv(const char *name, unsigned int hook_version, if (ret < 0) { SYSERROR("Failed to set environment " "variable: LXC_NET_PARENT=%s", parent); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_NET_PARENT=%s", parent); } else if (strcmp(argsin[0], "phys")) { @@ -446,7 +450,7 @@ int run_script_argv(const char *name, unsigned int hook_version, if (ret < 0) { SYSERROR("Failed to set environment " "variable: LXC_NET_PARENT=%s", parent); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_NET_PARENT=%s", parent); } else if (strcmp(argsin[0], "veth")) { @@ -456,7 +460,7 @@ int run_script_argv(const char *name, unsigned int hook_version, if (ret < 0) { SYSERROR("Failed to set environment " "variable: LXC_NET_PEER=%s", peer); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_NET_PEER=%s", peer); @@ -464,7 +468,7 @@ int run_script_argv(const char *name, unsigned int hook_version, if (ret < 0) { SYSERROR("Failed to set environment " "variable: LXC_NET_PARENT=%s", parent); - return -1; + goto on_error; } TRACE("Set environment variable: LXC_NET_PARENT=%s", parent); } @@ -477,12 +481,16 @@ int run_script_argv(const char *name, unsigned int hook_version, ret = snprintf(buffer + buf_pos, len, " %s", argsin[i]); if (ret < 0 || (size_t)ret >= len) { ERROR("Failed to create command line for script \"%s\"", script); - return -1; + goto on_error; } buf_pos += ret; } - return run_buffer(buffer); + fret = run_buffer(buffer); + +on_error: + free(buffer); + return fret; } int run_script(const char *name, const char *section, const char *script, ...) From 586b1ce72bc8a262b508109b6ad0a062c1ea9819 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 16 Feb 2018 22:11:19 +0100 Subject: [PATCH 3/5] conf: s/argsin/argv/ in run_script_argv() Signed-off-by: Christian Brauner --- src/lxc/conf.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/lxc/conf.c b/src/lxc/conf.c index 8995e0c16..e03a096c1 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -354,7 +354,7 @@ static int run_buffer(char *buffer) int run_script_argv(const char *name, unsigned int hook_version, const char *section, const char *script, - const char *hookname, char **argsin) + const char *hookname, char **argv) { int buf_pos, i, ret; char *buffer; @@ -367,8 +367,8 @@ int run_script_argv(const char *name, unsigned int hook_version, else INFO("Executing script \"%s\" for container \"%s\"", script, name); - for (i = 0; argsin && argsin[i]; i++) - size += strlen(argsin[i]) + 1; + for (i = 0; argv && argv[i]; i++) + size += strlen(argv[i]) + 1; size += sizeof("exec"); size += strlen(script); @@ -424,20 +424,20 @@ int run_script_argv(const char *name, unsigned int hook_version, if (strcmp(section, "net") == 0) { char *parent; - if (!argsin[0]) + if (!argv || !argv[0]) goto on_error; - ret = setenv("LXC_NET_TYPE", argsin[0], 1); + ret = setenv("LXC_NET_TYPE", argv[0], 1); if (ret < 0) { SYSERROR("Failed to set environment variable: " - "LXC_NET_TYPE=%s", argsin[0]); + "LXC_NET_TYPE=%s", argv[0]); goto on_error; } - TRACE("Set environment variable: LXC_NET_TYPE=%s", argsin[0]); + TRACE("Set environment variable: LXC_NET_TYPE=%s", argv[0]); - parent = argsin[1] ? argsin[1] : ""; + parent = argv[1] ? argv[1] : ""; - if (strcmp(argsin[0], "macvlan")) { + if (strcmp(argv[0], "macvlan")) { ret = setenv("LXC_NET_PARENT", parent, 1); if (ret < 0) { SYSERROR("Failed to set environment " @@ -445,7 +445,7 @@ int run_script_argv(const char *name, unsigned int hook_version, goto on_error; } TRACE("Set environment variable: LXC_NET_PARENT=%s", parent); - } else if (strcmp(argsin[0], "phys")) { + } else if (strcmp(argv[0], "phys")) { ret = setenv("LXC_NET_PARENT", parent, 1); if (ret < 0) { SYSERROR("Failed to set environment " @@ -453,8 +453,8 @@ int run_script_argv(const char *name, unsigned int hook_version, goto on_error; } TRACE("Set environment variable: LXC_NET_PARENT=%s", parent); - } else if (strcmp(argsin[0], "veth")) { - char *peer = argsin[2] ? argsin[2] : ""; + } else if (strcmp(argv[0], "veth")) { + char *peer = argv[2] ? argv[2] : ""; ret = setenv("LXC_NET_PEER", peer, 1); if (ret < 0) { @@ -475,10 +475,10 @@ int run_script_argv(const char *name, unsigned int hook_version, } } - for (i = 0; argsin && argsin[i]; i++) { + for (i = 0; argv && argv[i]; i++) { size_t len = size - buf_pos; - ret = snprintf(buffer + buf_pos, len, " %s", argsin[i]); + ret = snprintf(buffer + buf_pos, len, " %s", argv[i]); if (ret < 0 || (size_t)ret >= len) { ERROR("Failed to create command line for script \"%s\"", script); goto on_error; From 0d0d3655167f9658d0e19f775cfd20cdc52cd906 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 16 Feb 2018 23:18:54 +0100 Subject: [PATCH 4/5] start: don't call close on invalid file descriptor Signed-off-by: Christian Brauner --- src/lxc/start.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/start.c b/src/lxc/start.c index b90d5ea00..a64f6b64c 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -628,7 +628,7 @@ void lxc_free_handler(struct lxc_handler *handler) lxc_put_nsfds(handler); if (handler->conf && handler->conf->reboot == 0) - if (handler->conf->maincmd_fd) + if (handler->conf->maincmd_fd >= 0) close(handler->conf->maincmd_fd); if (handler->state_socket_pair[0] >= 0) From c06a0555e9cea7984eb1584c82ba310c2924956f Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Sat, 17 Feb 2018 00:04:30 +0100 Subject: [PATCH 5/5] console: ensure that fd is marked EBADF If the handler closes the file descriptor for the peer or master fd it is crucial that we mark it as -EBADF. This will prevent lxc_console_delete() from calling close() on an already closed file descriptor again. I've observed the double close in the attach code. Signed-off-by: Christian Brauner --- src/lxc/console.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/lxc/console.c b/src/lxc/console.c index a16466ba3..9168cdf9d 100644 --- a/src/lxc/console.c +++ b/src/lxc/console.c @@ -215,16 +215,20 @@ int lxc_console_cb_con(int fd, uint32_t events, void *data, if (r <= 0) { INFO("Console client on fd %d has exited", fd); lxc_mainloop_del_handler(descr, fd); - if (fd == console->peer) { + + if (fd == console->master) { + console->master = -EBADF; + } else if (fd == console->peer) { if (console->tty_state) { lxc_console_signal_fini(console->tty_state); console->tty_state = NULL; } - console->peer = -1; - close(fd); - return 0; + console->peer = -EBADF; + } else { + ERROR("Handler received unexpected file descriptor"); } close(fd); + return LXC_MAINLOOP_CLOSE; }