From 1703489bd31bbb5280f55571e8abe2b6f32bcbb3 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Mon, 30 Jul 2018 16:24:39 +0200 Subject: [PATCH 1/5] daemonize: use a safe umask for the daemon The reason for setting umask in a daemon is to get a defined umask value instead of whatever the calling user had configured. A umask of zero is dangerous, however, because it can easily cause world-readable and world-writeable files when oblivious code is involved that specified 0777 during open() calls, wanting to grant the user full control of the resulting file mode. Currently the daemon shouldn't be creating any new files so this is not a matter. This could change in the future, however. --- daemon/daemonize.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/daemonize.c b/daemon/daemonize.c index 5a68cbf..7d796a8 100644 --- a/daemon/daemonize.c +++ b/daemon/daemonize.c @@ -61,7 +61,7 @@ void daemonize(const char *name) } /* Now continue execution */ - umask(0); + umask(0022); if (setsid() < 0) { FATAL_ERRORNO("Failed to create process group\n"); } From 17241deccb3bd1bd59300048fbc7ae4ae07d1efc Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Mon, 30 Jul 2018 16:29:08 +0200 Subject: [PATCH 2/5] daemonize: instead of just closing STD*_FILENO replace them by /dev/null When there are not valid standard file descriptors then strange things can happen. When new file descriptors are opened, they will take the place of the former standard file descriptors and when e.g. somebody calls printf() they will write to some file descriptor that is not prepared for it. This would already happen during PLOG_MSG() in gamemoded. Actually this also causes a SIGABRT when calling gamemoded like this: ```bash gamemoded -d ``` This is due to a bug [1] in systemd that causes an assertion to be triggered. This shows that file descriptor zero is in this case being replaced by a UNIX domain socket representing the connection to the D-Bus session bus. Therefore instead of just closing the standard file descriptors, replace them by appropriate file descriptors refering to /dev/null. [1]: https://github.com/systemd/systemd/issues/8080 --- daemon/daemonize.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/daemon/daemonize.c b/daemon/daemonize.c index 7d796a8..cc475f3 100644 --- a/daemon/daemonize.c +++ b/daemon/daemonize.c @@ -31,6 +31,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "daemonize.h" #include "logging.h" +#include #include #include #include @@ -68,7 +69,13 @@ void daemonize(const char *name) if (chdir("/") < 0) { FATAL_ERRORNO("Failed to change to root directory\n"); } - close(STDIN_FILENO); - close(STDOUT_FILENO); - close(STDERR_FILENO); + + /* replace standard file descriptors by /dev/null */ + int devnull_r = open("/dev/null", O_RDONLY); + int devnull_w = open("/dev/null", O_WRONLY); + dup2(devnull_r, STDIN_FILENO); + dup2(devnull_w, STDOUT_FILENO); + dup2(devnull_w, STDERR_FILENO); + close(devnull_r); + close(devnull_w); } From 39d7b95a26a73aadda7a3b321809f3a1b3cfc3d0 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Mon, 30 Jul 2018 16:37:02 +0200 Subject: [PATCH 3/5] game_mode_context_register: fix memory leak on error conditions Clients can reach this memory leak by trying to unregister non-existing clients via D-Bus. --- daemon/gamemode.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 9a8380d..63c02f7 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -381,6 +381,12 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) /* Construct a new client if we can */ GameModeClient *cl = NULL; + /* Cap the total number of active clients */ + if (game_mode_context_num_clients(self) + 1 > MAX_GAMES) { + LOG_ERROR("Max games (%d) reached, not registering %d\n", MAX_GAMES, client); + return false; + } + cl = game_mode_client_new(client); if (!cl) { fputs("OOM\n", stderr); @@ -390,22 +396,16 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) if (game_mode_context_has_client(self, client)) { LOG_ERROR("Addition requested for already known process [%d]\n", client); - return false; - } - - /* Cap the total number of active clients */ - if (game_mode_context_num_clients(self) + 1 > MAX_GAMES) { - LOG_ERROR("Max games (%d) reached, not registering %d\n", MAX_GAMES, client); - return false; + goto error_cleanup; } /* Check our blacklist and whitelist */ if (!config_get_client_whitelisted(self->config, cl->executable)) { LOG_MSG("Client [%s] was rejected (not in whitelist)\n", cl->executable); - return false; + goto error_cleanup; } else if (config_get_client_blacklisted(self->config, cl->executable)) { LOG_MSG("Client [%s] was rejected (in blacklist)\n", cl->executable); - return false; + goto error_cleanup; } /* Begin a write lock now to insert our new client at list start */ @@ -427,6 +427,9 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) game_mode_apply_scheduler(self, client); return true; +error_cleanup: + game_mode_client_free(cl); + return false; } bool game_mode_context_unregister(GameModeContext *self, pid_t client) From 2896506340cee592c9e9645db83af67c5b3093cd Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Mon, 30 Jul 2018 16:43:58 +0200 Subject: [PATCH 4/5] cpugovctl: correctly evaluate available command line parameters When calling `cpugovctl set` then an access to the terminating NULL pointer of argv is made. --- daemon/cpugovctl.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/daemon/cpugovctl.c b/daemon/cpugovctl.c index 51b2e58..5aaf1fb 100644 --- a/daemon/cpugovctl.c +++ b/daemon/cpugovctl.c @@ -64,14 +64,9 @@ static void set_gov_state(const char *value) */ int main(int argc, char *argv[]) { - if (argc < 2) { - fprintf(stderr, "usage: cpugovctl [get] [set VALUE]\n"); - return EXIT_FAILURE; - } - - if (strncmp(argv[1], "get", 3) == 0) { + if (argc == 2 && strncmp(argv[1], "get", 3) == 0) { printf("%s", get_gov_state()); - } else if (strncmp(argv[1], "set", 3) == 0) { + } else if (argc == 3 && strncmp(argv[1], "set", 3) == 0) { const char *value = argv[2]; /* Must be root to set the state */ @@ -82,6 +77,7 @@ int main(int argc, char *argv[]) set_gov_state(value); } else { + fprintf(stderr, "usage: cpugovctl [get] [set VALUE]\n"); return EXIT_FAILURE; } From 5f06435bdf8678b4c9b268d9275891f2b8391693 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Mon, 30 Jul 2018 17:17:51 +0200 Subject: [PATCH 5/5] cpugovctl: evaluate govenor write errors and adjust exit code accordingly --- daemon/cpugovctl.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/daemon/cpugovctl.c b/daemon/cpugovctl.c index 5aaf1fb..b64b5b4 100644 --- a/daemon/cpugovctl.c +++ b/daemon/cpugovctl.c @@ -35,15 +35,18 @@ POSSIBILITY OF SUCH DAMAGE. #include "logging.h" #include +#include #include /** * Sets all governors to a value */ -static void set_gov_state(const char *value) +static int set_gov_state(const char *value) { char governors[MAX_GOVERNORS][MAX_GOVERNOR_LENGTH] = { { 0 } }; int num = fetch_governors(governors); + int retval = EXIT_SUCCESS; + int res = 0; LOG_MSG("Setting governors to %s\n", value); for (int i = 0; i < num; i++) { @@ -54,9 +57,15 @@ static void set_gov_state(const char *value) continue; } - fprintf(f, "%s\n", value); + res = fprintf(f, "%s\n", value); + if (res < 0) { + LOG_ERROR("Failed to set governor %s to %s: %s", gov, value, strerror(errno)); + retval = EXIT_FAILURE; + } fclose(f); } + + return retval; } /** @@ -75,7 +84,7 @@ int main(int argc, char *argv[]) return EXIT_FAILURE; } - set_gov_state(value); + return set_gov_state(value); } else { fprintf(stderr, "usage: cpugovctl [get] [set VALUE]\n"); return EXIT_FAILURE;