From 7eaee902862433ffa45f0e73046226830ac33ab1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 12 May 2021 13:45:01 +0200 Subject: [PATCH 1/6] terminal-util: add debug logging for when TTY ioctls fail --- src/basic/terminal-util.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index fafdaaa090..91c404892e 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -245,17 +245,20 @@ int reset_terminal_fd(int fd, bool switch_to_text) { * interfere with that. */ /* Disable exclusive mode, just in case */ - (void) ioctl(fd, TIOCNXCL); + if (ioctl(fd, TIOCNXCL) < 0) + log_debug_errno(errno, "TIOCNXCL ioctl failed on TTY, ignoring: %m"); /* Switch to text mode */ if (switch_to_text) - (void) ioctl(fd, KDSETMODE, KD_TEXT); + if (ioctl(fd, KDSETMODE, KD_TEXT) < 0) + log_debug_errno(errno, "KDSETMODE ioctl for switching to text mode failed on TTY, ignoring: %m"); + /* Set default keyboard mode */ (void) vt_reset_keyboard(fd); if (tcgetattr(fd, &termios) < 0) { - r = -errno; + r = log_debug_errno(errno, "Failed to get terminal parameters: %m"); goto finish; } From e60a4a3c46df1abc6570a026d716c71a6cb1bf5c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 12 May 2021 13:45:48 +0200 Subject: [PATCH 2/6] terminal-util: add extra validity checks that we operate on a TTY before doing so Prompted by #19213, but not fixing it. This is mostly paranoia that we don't do stuff on inodes that aren't actually ttys. --- src/basic/terminal-util.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 91c404892e..9bc6bb2024 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -240,9 +240,11 @@ int reset_terminal_fd(int fd, bool switch_to_text) { assert(fd >= 0); - /* We leave locked terminal attributes untouched, so that - * Plymouth may set whatever it wants to set, and we don't - * interfere with that. */ + if (isatty(fd) < 1) + return log_debug_errno(errno, "Asked to reset a terminal that actually isn't a terminal: %m"); + + /* We leave locked terminal attributes untouched, so that Plymouth may set whatever it wants to set, + * and we don't interfere with that. */ /* Disable exclusive mode, just in case */ if (ioctl(fd, TIOCNXCL) < 0) @@ -1329,6 +1331,9 @@ int vt_restore(int fd) { }; int r, q = 0; + if (isatty(fd) < 1) + return log_debug_errno(errno, "Asked to restore the VT for an fd that does not refer to a terminal: %m"); + if (ioctl(fd, KDSETMODE, KD_TEXT) < 0) q = log_debug_errno(errno, "Failed to set VT in text mode, ignoring: %m"); @@ -1362,6 +1367,9 @@ int vt_release(int fd, bool restore) { * sent by the kernel and optionally reset the VT in text and auto * VT-switching modes. */ + if (isatty(fd) < 1) + return log_debug_errno(errno, "Asked to release the VT for an fd that does not refer to a terminal: %m"); + if (ioctl(fd, VT_RELDISP, 1) < 0) return -errno; From 4768529ff1eb41b011f2c8fcb36c5ae712371d0f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 12 May 2021 13:47:45 +0200 Subject: [PATCH 3/6] terminal-util: use _cleanup_close_ where appropriate --- src/basic/terminal-util.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 9bc6bb2024..1b3e551204 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -316,14 +316,13 @@ int reset_terminal(const char *name) { } int open_terminal(const char *name, int mode) { + _cleanup_close_ int fd = -1; unsigned c = 0; - int fd; /* - * If a TTY is in the process of being closed opening it might - * cause EIO. This is horribly awful, but unlikely to be - * changed in the kernel. Hence we work around this problem by - * retrying a couple of times. + * If a TTY is in the process of being closed opening it might cause EIO. This is horribly awful, but + * unlikely to be changed in the kernel. Hence we work around this problem by retrying a couple of + * times. * * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/554172/comments/245 */ @@ -343,16 +342,14 @@ int open_terminal(const char *name, int mode) { if (c >= 20) return -errno; - usleep(50 * USEC_PER_MSEC); + (void) usleep(50 * USEC_PER_MSEC); c++; } - if (isatty(fd) <= 0) { - safe_close(fd); - return -ENOTTY; - } + if (isatty(fd) < 1) + return negative_errno(); - return fd; + return TAKE_FD(fd); } int acquire_terminal( From f2df231fed18df3cb46a62352a089bd9ba296601 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 12 May 2021 13:49:37 +0200 Subject: [PATCH 4/6] core: use GID_INVALID instead of -1 where appropriate --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index 6a1e6c8429..2ee85c5e63 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -744,7 +744,7 @@ static int chown_terminal(int fd, uid_t uid) { } /* This might fail. What matters are the results. */ - r = fchmod_and_chown(fd, TTY_MODE, uid, -1); + r = fchmod_and_chown(fd, TTY_MODE, uid, GID_INVALID); if (r < 0) return r; From 0ba976e8da8f4878e478d02468a93977a9d3ee35 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 12 May 2021 14:00:07 +0200 Subject: [PATCH 5/6] execute: don't chown/chmod non-TTY inodes thinking they were TTYs Fixes: #19213 This is a safety net for invalid configurations, see the original bug report. --- src/core/execute.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 2ee85c5e63..a16955cbd9 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -5775,6 +5775,9 @@ void exec_context_free_log_extra_fields(ExecContext *c) { } void exec_context_revert_tty(ExecContext *c) { + _cleanup_close_ int fd = -1; + const char *path; + struct stat st; int r; assert(c); @@ -5785,17 +5788,33 @@ void exec_context_revert_tty(ExecContext *c) { /* And then undo what chown_terminal() did earlier. Note that we only do this if we have a path * configured. If the TTY was passed to us as file descriptor we assume the TTY is opened and managed * by whoever passed it to us and thus knows better when and how to chmod()/chown() it back. */ + if (!exec_context_may_touch_tty(c)) + return; - if (exec_context_may_touch_tty(c)) { - const char *path; + path = exec_context_tty_path(c); + if (!path) + return; - path = exec_context_tty_path(c); - if (path) { - r = chmod_and_chown(path, TTY_MODE, 0, TTY_GID); - if (r < 0 && r != -ENOENT) - log_warning_errno(r, "Failed to reset TTY ownership/access mode of %s, ignoring: %m", path); - } - } + fd = open(path, O_PATH|O_CLOEXEC); + if (fd < 0) + return (void) log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, errno, + "Failed to open TTY inode of '%s' to adjust ownership/access mode, ignoring: %m", + path); + + if (fstat(fd, &st) < 0) + return (void) log_warning_errno(errno, "Failed to stat TTY '%s', ignoring: %m", path); + + /* Let's add a superficial check that we only do this for stuff that looks like a TTY. We only check + * if things are a character device, since a proper check either means we'd have to open the TTY and + * use isatty(), but we'd rather not do that since opening TTYs comes with all kinds of side-effects + * and is slow. Or we'd have to hardcode dev_t major information, which we'd rather avoid. Why bother + * with this at all? → https://github.com/systemd/systemd/issues/19213 */ + if (!S_ISCHR(st.st_mode)) + return log_warning("Configured TTY '%s' is not actually a character device, ignoring.", path); + + r = fchmod_and_chown(fd, TTY_MODE, 0, TTY_GID); + if (r < 0) + log_warning_errno(r, "Failed to reset TTY ownership/access mode of %s, ignoring: %m", path); } int exec_context_get_clean_directories( From 11f3c130aa6a61021f2e4106609df4d052e28a52 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 12 May 2021 16:05:40 +0200 Subject: [PATCH 6/6] terminal: don't hardcode major number of PTYs Hardcoding major numbers sucks. And we generally don't do it, except when determining whether something is a PTY. Thing though is that we don't actually need to do that here either, hence don#t. --- src/basic/terminal-util.c | 64 +++++++++++++++++++++-------------- src/test/test-terminal-util.c | 25 ++++++++++++++ 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 1b3e551204..ed0632b02b 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -37,6 +37,7 @@ #include "process-util.h" #include "socket-util.h" #include "stat-util.h" +#include "stdio-util.h" #include "string-util.h" #include "strv.h" #include "terminal-util.h" @@ -962,7 +963,9 @@ int get_ctty_devnr(pid_t pid, dev_t *d) { } int get_ctty(pid_t pid, dev_t *ret_devnr, char **ret) { - _cleanup_free_ char *fn = NULL, *b = NULL; + char pty[STRLEN("/dev/pts/") + DECIMAL_STR_MAX(dev_t) + 1]; + _cleanup_free_ char *buf = NULL; + const char *fn = NULL, *w; dev_t devnr; int r; @@ -970,44 +973,53 @@ int get_ctty(pid_t pid, dev_t *ret_devnr, char **ret) { if (r < 0) return r; - r = device_path_make_canonical(S_IFCHR, devnr, &fn); + r = device_path_make_canonical(S_IFCHR, devnr, &buf); if (r < 0) { + struct stat st; + if (r != -ENOENT) /* No symlink for this in /dev/char/? */ return r; - if (major(devnr) == 136) { - /* This is an ugly hack: PTY devices are not listed in /dev/char/, as they don't follow the - * Linux device model. This means we have no nice way to match them up against their actual - * device node. Let's hence do the check by the fixed, assigned major number. Normally we try - * to avoid such fixed major/minor matches, but there appears to nother nice way to handle - * this. */ + /* Maybe this is PTY? PTY devices are not listed in /dev/char/, as they don't follow the + * Linux device model and hence device_path_make_canonical() doesn't work for them. Let's + * assume this is a PTY for a moment, and check if the device node this would then map to in + * /dev/pts/ matches the one we are looking for. This way we don't have to hardcode the major + * number (which is 136 btw), but we still rely on the fact that PTY numbers map directly to + * the minor number of the pty. */ + xsprintf(pty, "/dev/pts/%u", minor(devnr)); - if (asprintf(&b, "pts/%u", minor(devnr)) < 0) - return -ENOMEM; - } else { - /* Probably something similar to the ptys which have no symlink in /dev/char/. Let's return - * something vaguely useful. */ + if (stat(pty, &st) < 0) { + if (errno != ENOENT) + return -errno; - r = device_path_make_major_minor(S_IFCHR, devnr, &fn); + } else if (S_ISCHR(st.st_mode) && devnr == st.st_rdev) /* Bingo! */ + fn = pty; + + if (!fn) { + /* Doesn't exist, or not a PTY? Probably something similar to the PTYs which have no + * symlink in /dev/char/. Let's return something vaguely useful. */ + r = device_path_make_major_minor(S_IFCHR, devnr, &buf); if (r < 0) return r; + + fn = buf; } - } + } else + fn = buf; - if (!b) { - const char *w; + w = path_startswith(fn, "/dev/"); + if (!w) + return -EINVAL; - w = path_startswith(fn, "/dev/"); - if (w) { - b = strdup(w); - if (!b) - return -ENOMEM; - } else - b = TAKE_PTR(fn); - } + if (ret) { + _cleanup_free_ char *b = NULL; + + b = strdup(w); + if (!b) + return -ENOMEM; - if (ret) *ret = TAKE_PTR(b); + } if (ret_devnr) *ret_devnr = devnr; diff --git a/src/test/test-terminal-util.c b/src/test/test-terminal-util.c index 508f0c03ee..e793a74b13 100644 --- a/src/test/test-terminal-util.c +++ b/src/test/test-terminal-util.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "alloc-util.h" @@ -153,6 +154,29 @@ static void test_text(void) { } } +static void test_get_ctty(void) { + _cleanup_free_ char *ctty = NULL; + struct stat st; + dev_t devnr; + int r; + + r = get_ctty(0, &devnr, &ctty); + if (r < 0) { + log_notice_errno(r, "Apparently called without a controlling TTY, cutting get_ctty() test short: %m"); + return; + } + + /* In almost all cases STDIN will match our controlling TTY. Let's verify that and then compare paths */ + assert_se(fstat(STDIN_FILENO, &st) >= 0); + if (S_ISCHR(st.st_mode) && st.st_rdev == devnr) { + _cleanup_free_ char *stdin_name = NULL; + + assert_se(getttyname_malloc(STDIN_FILENO, &stdin_name) >= 0); + assert_se(path_equal(stdin_name, ctty)); + } else + log_notice("Not invoked with stdin == ctty, cutting get_ctty() test short"); +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); @@ -161,6 +185,7 @@ int main(int argc, char *argv[]) { test_getttyname_malloc(); test_colors(); test_text(); + test_get_ctty(); return 0; }