From 37232d55a7bcace37280e28b207c85f5ca9b3f6b Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 26 Apr 2023 14:18:04 +0100 Subject: [PATCH 1/3] coredump filter: fix stack overflow with =all We translate 'all' to UNIT64_MAX, which has a lot more 'f's. Use the helper macro, since a decimal uint64_t will always be >> than a hex representation. root@image:~# systemd-run -t --property CoredumpFilter=all ls /tmp Running as unit: run-u13.service Press ^] three times within 1s to disconnect TTY. *** stack smashing detected ***: terminated [137256.320511] systemd[1]: run-u13.service: Main process exited, code=dumped, status=6/ABRT [137256.320850] systemd[1]: run-u13.service: Failed with result 'core-dump'. --- src/basic/macro.h | 4 ++++ src/shared/coredump-util.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/basic/macro.h b/src/basic/macro.h index 99262e92065..13620a60bca 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -250,6 +250,10 @@ static inline int __coverity_check_and_return__(int condition) { #define sizeof_field(struct_type, member) sizeof(((struct_type *) 0)->member) #define endoffsetof_field(struct_type, member) (offsetof(struct_type, member) + sizeof_field(struct_type, member)) +/* Maximum buffer size needed for formatting an unsigned integer type as hex, including space for '0x' + * prefix and trailing NUL suffix. */ +#define HEXADECIMAL_STR_MAX(type) (2 + sizeof(type) * 2 + 1) + /* Returns the number of chars needed to format variables of the specified type as a decimal string. Adds in * extra space for a negative '-' prefix for signed types. Includes space for the trailing NUL. */ #define DECIMAL_STR_MAX(type) \ diff --git a/src/shared/coredump-util.c b/src/shared/coredump-util.c index bf8ea00b14c..dc137317a90 100644 --- a/src/shared/coredump-util.c +++ b/src/shared/coredump-util.c @@ -158,9 +158,9 @@ int parse_auxv(int log_level, } int set_coredump_filter(uint64_t value) { - char t[STRLEN("0xFFFFFFFF")]; + char t[HEXADECIMAL_STR_MAX(uint64_t)]; - sprintf(t, "0x%"PRIx64, value); + xsprintf(t, "0x%"PRIx64, value); return write_string_file("/proc/self/coredump_filter", t, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_DISABLE_BUFFER); From 7f3bb8f20dcccaceea8b1ee05f0560b81162037b Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 26 Apr 2023 14:19:33 +0100 Subject: [PATCH 2/3] coredump filter: add mask for 'all' using UINT32_MAX, not UINT64_MAX The kernel returns ERANGE when UINT64_MAX is passed. Create a mask and use UINT32_max, which is accepted, so that future bits will also be set. --- src/shared/coredump-util.c | 2 +- src/shared/coredump-util.h | 3 +++ src/test/test-coredump-util.c | 2 ++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/shared/coredump-util.c b/src/shared/coredump-util.c index dc137317a90..805503f366a 100644 --- a/src/shared/coredump-util.c +++ b/src/shared/coredump-util.c @@ -46,7 +46,7 @@ int coredump_filter_mask_from_string(const char *s, uint64_t *ret) { } if (streq(n, "all")) { - m = UINT64_MAX; + m = COREDUMP_FILTER_MASK_ALL; continue; } diff --git a/src/shared/coredump-util.h b/src/shared/coredump-util.h index 99dbfde730e..4f54bb94c00 100644 --- a/src/shared/coredump-util.h +++ b/src/shared/coredump-util.h @@ -22,6 +22,9 @@ typedef enum CoredumpFilter { 1u << COREDUMP_FILTER_ELF_HEADERS | \ 1u << COREDUMP_FILTER_PRIVATE_HUGE) +/* The kernel doesn't like UINT64_MAX and returns ERANGE, use UINT32_MAX to support future new flags */ +#define COREDUMP_FILTER_MASK_ALL UINT32_MAX + const char* coredump_filter_to_string(CoredumpFilter i) _const_; CoredumpFilter coredump_filter_from_string(const char *s) _pure_; int coredump_filter_mask_from_string(const char *s, uint64_t *ret); diff --git a/src/test/test-coredump-util.c b/src/test/test-coredump-util.c index 7a41e0fc294..178e89389b1 100644 --- a/src/test/test-coredump-util.c +++ b/src/test/test-coredump-util.c @@ -28,6 +28,8 @@ TEST(coredump_filter_mask_from_string) { uint64_t f; assert_se(coredump_filter_mask_from_string("default", &f) == 0); assert_se(f == COREDUMP_FILTER_MASK_DEFAULT); + assert_se(coredump_filter_mask_from_string("all", &f) == 0); + assert_se(f == COREDUMP_FILTER_MASK_ALL); assert_se(coredump_filter_mask_from_string(" default\tdefault\tdefault ", &f) == 0); assert_se(f == COREDUMP_FILTER_MASK_DEFAULT); From cf636aa59eb8c848ed04d5b08aac0acf3f6683d9 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 26 Apr 2023 14:32:04 +0100 Subject: [PATCH 3/3] test: add coverage for CoredumpFilter=all --- test/units/testsuite-74.coredump.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/units/testsuite-74.coredump.sh b/test/units/testsuite-74.coredump.sh index 3910abe0ec1..0e5d050f45e 100755 --- a/test/units/testsuite-74.coredump.sh +++ b/test/units/testsuite-74.coredump.sh @@ -153,6 +153,9 @@ timeout 30 bash -c "while [[ \$(coredumpctl list -q --no-legend $$ | wc -l) -eq coredumpctl info "$$" coredumpctl info COREDUMP_HOSTNAME="mymachine" +# This used to cause a stack overflow +systemd-run -t --property CoredumpFilter=all ls /tmp +systemd-run -t --property CoredumpFilter=default ls /tmp (! coredumpctl --hello-world) (! coredumpctl -n 0)