From de699c7a06797618c008ad3ca13bab34186298bc Mon Sep 17 00:00:00 2001 From: David Tardon Date: Wed, 3 Oct 2018 12:48:58 +0200 Subject: [PATCH 1/9] console: avoid promotion to signed int coverity message: sign_extension: Suspicious implicit sign extension: "keydata.Key.ScanCode" with type "UINT16" (16 bits, unsigned) is promoted in "keydata.Key.ScanCode << 16" to type "int" (32 bits, signed), then sign-extended to type "unsigned long" (64 bits, unsigned). If "keydata.Key.ScanCode << 16" is greater than 0x7FFFFFFF, the upper bits of the result will all be 1. --- src/boot/efi/console.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/efi/console.h b/src/boot/efi/console.h index 10c5ce4ebd..b9ed6c70b3 100644 --- a/src/boot/efi/console.h +++ b/src/boot/efi/console.h @@ -9,7 +9,7 @@ #define EFI_CONTROL_PRESSED (EFI_RIGHT_CONTROL_PRESSED|EFI_LEFT_CONTROL_PRESSED) #define EFI_ALT_PRESSED (EFI_RIGHT_ALT_PRESSED|EFI_LEFT_ALT_PRESSED) -#define KEYPRESS(keys, scan, uni) ((((UINT64)keys) << 32) | ((scan) << 16) | (uni)) +#define KEYPRESS(keys, scan, uni) ((((UINT64)keys) << 32) | (((UINT64)scan) << 16) | (uni)) #define KEYCHAR(k) ((k) & 0xffff) #define CHAR_CTRL(c) ((c) - 'a' + 1) From 4db1879acdc0b853e1a7e6e650b6feb917175fac Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 9 Oct 2018 13:50:55 +0200 Subject: [PATCH 2/9] dissect-image: use right comparison function fstype can be NULL here. --- src/shared/dissect-image.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index b6e0534b5d..710507ed5c 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -231,7 +231,7 @@ int dissect_image( .node = TAKE_PTR(n), }; - m->encrypted = streq(fstype, "crypto_LUKS"); + m->encrypted = streq_ptr(fstype, "crypto_LUKS"); *ret = TAKE_PTR(m); From f369f47c264dd56f630e91697006efd647542feb Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 9 Oct 2018 15:04:58 +0200 Subject: [PATCH 3/9] be consistent about sun_path length Most places use the whole buffer for name, without leaving extra space for the trailing NUL. --- src/core/manager.c | 2 +- src/login/pam_systemd.c | 2 +- src/nspawn/nspawn.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 2dea89dc01..446648a265 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -892,7 +892,7 @@ static int manager_setup_notify(Manager *m) { (void) mkdir_parents_label(m->notify_socket, 0755); (void) unlink(m->notify_socket); - strncpy(sa.un.sun_path, m->notify_socket, sizeof(sa.un.sun_path)-1); + strncpy(sa.un.sun_path, m->notify_socket, sizeof(sa.un.sun_path)); r = bind(fd, &sa.sa, SOCKADDR_UN_LEN(sa.un)); if (r < 0) return log_error_errno(errno, "bind(%s) failed: %m", sa.un.sun_path); diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index 8464df122c..90ccbd7a83 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -160,7 +160,7 @@ static int get_seat_from_display(const char *display, const char **seat, uint32_ r = socket_from_display(display, &p); if (r < 0) return r; - strncpy(sa.un.sun_path, p, sizeof(sa.un.sun_path)-1); + strncpy(sa.un.sun_path, p, sizeof(sa.un.sun_path)); fd = socket(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); if (fd < 0) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 27547051c8..134c4c3b5d 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -2826,7 +2826,7 @@ static int setup_sd_notify_child(void) { (void) mkdir_parents(NSPAWN_NOTIFY_SOCKET_PATH, 0755); (void) unlink(NSPAWN_NOTIFY_SOCKET_PATH); - strncpy(sa.un.sun_path, NSPAWN_NOTIFY_SOCKET_PATH, sizeof(sa.un.sun_path)-1); + strncpy(sa.un.sun_path, NSPAWN_NOTIFY_SOCKET_PATH, sizeof(sa.un.sun_path)); r = bind(fd, &sa.sa, SOCKADDR_UN_LEN(sa.un)); if (r < 0) { safe_close(fd); From 8192548ef792c7aa3202a27e060a2c4994f399f3 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Fri, 12 Oct 2018 14:48:41 +0200 Subject: [PATCH 4/9] do not try to allocate 0 bytes --- src/shared/fdset.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/shared/fdset.c b/src/shared/fdset.c index 8c852f1137..5d277328c7 100644 --- a/src/shared/fdset.c +++ b/src/shared/fdset.c @@ -211,13 +211,16 @@ fail: int fdset_close_others(FDSet *fds) { void *e; Iterator i; - int *a; + int *a = NULL; size_t j = 0, m; m = fdset_size(fds); - a = newa(int, m); - SET_FOREACH(e, MAKE_SET(fds), i) - a[j++] = PTR_TO_FD(e); + + if (m > 0) { + a = newa(int, m); + SET_FOREACH(e, MAKE_SET(fds), i) + a[j++] = PTR_TO_FD(e); + } assert(j == m); From c05107767b589e9aac9711eb385738887f86eb77 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 9 Oct 2018 16:53:43 +0200 Subject: [PATCH 5/9] alloc-util: return NULL if 0-sized allocation is requested That would almost certainly be an error (e.g., an overflow in computing _need_), so it's better to fail. --- src/basic/alloc-util.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h index ebe42889ea..62b591117b 100644 --- a/src/basic/alloc-util.h +++ b/src/basic/alloc-util.h @@ -52,8 +52,11 @@ static inline void freep(void *p) { #define _cleanup_free_ _cleanup_(freep) +/* Checks the size arguments of allocation functions for overflow in multiplication. In addition, checks if either of + * them is 0; that is almost certainly an error (e.g., an overflow in computing _need_), so it's better to fail (and + * we cannot leave this check to malloc, because the behavior of malloc(0) is impl. specific). */ static inline bool size_multiply_overflow(size_t size, size_t need) { - return _unlikely_(need != 0 && size > (SIZE_MAX / need)); + return _unlikely_(need == 0 || size == 0 || size > (SIZE_MAX / need)); } _malloc_ _alloc_(1, 2) static inline void *malloc_multiply(size_t size, size_t need) { From e99742ef3e9d847da04e71fec0eb426063b25068 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 9 Oct 2018 17:26:19 +0200 Subject: [PATCH 6/9] login: avoid leak of name returned by uid_to_name() --- src/login/logind-dbus.c | 4 +++- src/login/logind-utmp.c | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 8e22538ac3..ed6da4445b 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2157,6 +2157,7 @@ static int method_cancel_scheduled_shutdown(sd_bus_message *message, void *userd if (cancelled && m->enable_wall_messages) { _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + _cleanup_free_ char *username = NULL; const char *tty = NULL; uid_t uid = 0; int r; @@ -2167,8 +2168,9 @@ static int method_cancel_scheduled_shutdown(sd_bus_message *message, void *userd (void) sd_bus_creds_get_tty(creds, &tty); } + username = uid_to_name(uid); utmp_wall("The system shutdown has been cancelled", - uid_to_name(uid), tty, logind_wall_tty_filter, m); + username, tty, logind_wall_tty_filter, m); } return sd_bus_reply_method_return(message, "b", cancelled); diff --git a/src/login/logind-utmp.c b/src/login/logind-utmp.c index 71ebdfcfb1..8bdd4ab6bf 100644 --- a/src/login/logind-utmp.c +++ b/src/login/logind-utmp.c @@ -61,7 +61,7 @@ bool logind_wall_tty_filter(const char *tty, void *userdata) { static int warn_wall(Manager *m, usec_t n) { char date[FORMAT_TIMESTAMP_MAX] = {}; - _cleanup_free_ char *l = NULL; + _cleanup_free_ char *l = NULL, *username = NULL; usec_t left; int r; @@ -83,8 +83,8 @@ static int warn_wall(Manager *m, usec_t n) { return 0; } - utmp_wall(l, uid_to_name(m->scheduled_shutdown_uid), - m->scheduled_shutdown_tty, logind_wall_tty_filter, m); + username = uid_to_name(m->scheduled_shutdown_uid); + utmp_wall(l, username, m->scheduled_shutdown_tty, logind_wall_tty_filter, m); return 1; } From 0b777d20e9a3868b12372ffce8040d1be063cec7 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Wed, 10 Oct 2018 09:33:28 +0200 Subject: [PATCH 7/9] firewall-util: add an assert that we're not overwriting a buffer ... like commit f28501279d2c28fdbb31d8273b723e9bf71d3b98 does for out_interface. --- src/shared/firewall-util.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/shared/firewall-util.c b/src/shared/firewall-util.c index eb4f5ff616..cba52fb419 100644 --- a/src/shared/firewall-util.c +++ b/src/shared/firewall-util.c @@ -50,8 +50,14 @@ static int entry_fill_basics( entry->ip.proto = protocol; if (in_interface) { + size_t l; + + l = strlen(in_interface); + assert(l < sizeof entry->ip.iniface); + assert(l < sizeof entry->ip.iniface_mask); + strcpy(entry->ip.iniface, in_interface); - memset(entry->ip.iniface_mask, 0xFF, strlen(in_interface)+1); + memset(entry->ip.iniface_mask, 0xFF, l + 1); } if (source) { entry->ip.src = source->in; From c52368509f48e556be5a4c7a171361b656a25e02 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Wed, 10 Oct 2018 13:56:54 +0200 Subject: [PATCH 8/9] journal-file: avoid calling ftruncate with invalid fd This can happen if journal_file_close is called from the failure handling code of journal_file_open before f->fd was established. --- src/journal/journal-file.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index e9dddbc0d7..9c92d67516 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -1846,6 +1846,9 @@ static int journal_file_append_entry_internal( void journal_file_post_change(JournalFile *f) { assert(f); + if (f->fd < 0) + return; + /* inotify() does not receive IN_MODIFY events from file * accesses done via mmap(). After each access we hence * trigger IN_MODIFY by truncating the journal file to its From e7e36b90305d2e867259ba428512f02c1c11a0d8 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Wed, 10 Oct 2018 14:54:05 +0200 Subject: [PATCH 9/9] efivars: check path_len before using it as loop boundary --- src/shared/efivars.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/shared/efivars.c b/src/shared/efivars.c index 3931bee559..da70e68c81 100644 --- a/src/shared/efivars.c +++ b/src/shared/efivars.c @@ -412,9 +412,13 @@ int efi_get_boot_option( if (header->path_len > 0) { uint8_t *dbuf; - size_t dnext; + size_t dnext, doff; + + doff = offsetof(struct boot_option, title) + title_size; + dbuf = buf + doff; + if (header->path_len > l - doff) + return -EINVAL; - dbuf = buf + offsetof(struct boot_option, title) + title_size; dnext = 0; while (dnext < header->path_len) { struct device_path *dpath;