From d3d8597782f1976d21d156da2535abe5b1111c31 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Thu, 25 May 2023 22:47:13 +0200 Subject: [PATCH 1/3] journal-remote: bump the refcount right after creating the writer object Otherwise we might hit an assertion during cleanup if the following mmap_cache_new() call fails: Assertion 'p->n_ref > 0' failed at src/journal-remote/journal-remote-write.c:80, function writer_unref(). Aborting. ==2069==ERROR: AddressSanitizer: ABRT on unknown address 0x000000000815 (pc 0x7f39dcd0200b bp 0x7ffe2fe24db0 sp 0x7ffe2fe24b60 T0) SCARINESS: 10 (signal) #0 0x7f39dcd0200b in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) #1 0x7f39dcce1858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22858) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) #2 0x7f39dd747e49 in log_assert_failed /work/build/../../src/systemd/src/basic/log.c:940:9 #3 0x4e4431 in writer_unref /work/build/../../src/systemd/src/journal-remote/journal-remote-write.c:80:1 #4 0x4e3fd5 in writer_unrefp /work/build/../../src/systemd/src/journal-remote/journal-remote-write.h:27:1 #5 0x4e3fd5 in writer_new /work/build/../../src/systemd/src/journal-remote/journal-remote-write.c:56:1 #6 0x4e04bc in journal_remote_get_writer /work/build/../../src/systemd/src/journal-remote/journal-remote.c:125:21 #7 0x4e0e0b in get_source_for_fd /work/build/../../src/systemd/src/journal-remote/journal-remote.c:181:13 #8 0x4e0e0b in journal_remote_add_source /work/build/../../src/systemd/src/journal-remote/journal-remote.c:233:13 #9 0x4df99f in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/journal-remote/fuzz-journal-remote.c:54:9 #10 0x4e8f48 in NaloFuzzerTestOneInput (/build/fuzz-journal-remote+0x4e8f48) #11 0x507f53 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15 #12 0x50773a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3 #13 0x508e09 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:757:19 #14 0x509ad5 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:895:5 #15 0x4f8e3f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6 #16 0x4f9708 in LLVMFuzzerRunDriver /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:925:10 #17 0x4e91c5 in main (/build/fuzz-journal-remote+0x4e91c5) #18 0x7f39dcce3082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) #19 0x420bcd in _start (/build/fuzz-journal-remote+0x420bcd) DEDUP_TOKEN: raise--abort--log_assert_failed AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) in raise Found by Nallocufzz. --- src/journal-remote/journal-remote-write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal-remote/journal-remote-write.c b/src/journal-remote/journal-remote-write.c index 54ed1e23a0..2d8188ec62 100644 --- a/src/journal-remote/journal-remote-write.c +++ b/src/journal-remote/journal-remote-write.c @@ -32,13 +32,13 @@ int writer_new(RemoteServer *server, Writer **ret) { if (!w) return -ENOMEM; + w->n_ref = 1; w->metrics = server->metrics; w->mmap = mmap_cache_new(); if (!w->mmap) return -ENOMEM; - w->n_ref = 1; w->server = server; if (is_dir(server->output, /* follow = */ true) > 0) { From 8771463957f0fc12458aea8cc549d95183e4ee05 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Thu, 25 May 2023 11:58:32 +0200 Subject: [PATCH 2/3] sd-journal: log about errors from ordered_hashmap_*() Follow-up to 08a8fd6e8d. --- src/libsystemd/sd-journal/catalog.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-journal/catalog.c b/src/libsystemd/sd-journal/catalog.c index 06cf396b95..f490e21490 100644 --- a/src/libsystemd/sd-journal/catalog.c +++ b/src/libsystemd/sd-journal/catalog.c @@ -172,7 +172,7 @@ static int finish_item( r = ordered_hashmap_update(h, i, combined); if (r < 0) - return r; + return log_error_errno(r, "Failed to update catalog item: %m"); TAKE_PTR(combined); free(prev); @@ -184,7 +184,7 @@ static int finish_item( r = ordered_hashmap_put(h, i, combined); if (r < 0) - return r; + return log_error_errno(r, "Failed to insert catalog item: %m"); TAKE_PTR(i); TAKE_PTR(combined); From aca607d18d4324e921dc6c4e4190d9b496a4ac55 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Fri, 26 May 2023 11:38:58 +0200 Subject: [PATCH 3/3] sd-network: avoid leaking DHCPLease If we fail any allocation prior adding the lease to the server lease hashmap. ==2103==ERROR: LeakSanitizer: detected memory leaks Direct leak of 128 byte(s) in 2 object(s) allocated from: #0 0x4a203e in __interceptor_calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:77:3 #1 0x4f6341 in calloc (/build/fuzz-dhcp-server+0x4f6341) #2 0x4ec818 in add_lease /work/build/../../src/systemd/src/libsystemd-network/fuzz-dhcp-server.c:26:9 #3 0x4ec2bf in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/libsystemd-network/fuzz-dhcp-server.c:75:9 #4 0x4f68a8 in NaloFuzzerTestOneInput (/build/fuzz-dhcp-server+0x4f68a8) #5 0x5158b3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15 #6 0x51509a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3 #7 0x516769 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:757:19 #8 0x517435 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:895:5 #9 0x50679f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6 #10 0x507068 in LLVMFuzzerRunDriver /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:925:10 #11 0x4f6b25 in main (/build/fuzz-dhcp-server+0x4f6b25) #12 0x7f16084e3082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) DEDUP_TOKEN: __interceptor_calloc--calloc--add_lease SUMMARY: AddressSanitizer: 128 byte(s) leaked in 2 allocation(s). Found by Nallocufzz. --- src/libsystemd-network/dhcp-server-internal.h | 3 ++ src/libsystemd-network/fuzz-dhcp-server.c | 53 +++++++++++++------ src/libsystemd-network/sd-dhcp-server.c | 4 +- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/libsystemd-network/dhcp-server-internal.h b/src/libsystemd-network/dhcp-server-internal.h index 607f9f0c2f..466d4541c3 100644 --- a/src/libsystemd-network/dhcp-server-internal.h +++ b/src/libsystemd-network/dhcp-server-internal.h @@ -115,6 +115,9 @@ int dhcp_server_send_packet(sd_dhcp_server *server, void client_id_hash_func(const DHCPClientId *p, struct siphash *state); int client_id_compare_func(const DHCPClientId *a, const DHCPClientId *b); +DHCPLease *dhcp_lease_free(DHCPLease *lease); +DEFINE_TRIVIAL_CLEANUP_FUNC(DHCPLease*, dhcp_lease_free); + #define log_dhcp_server_errno(server, error, fmt, ...) \ log_interface_prefix_full_errno( \ "DHCPv4 server: ", \ diff --git a/src/libsystemd-network/fuzz-dhcp-server.c b/src/libsystemd-network/fuzz-dhcp-server.c index b35f1ac6da..ad00654a91 100644 --- a/src/libsystemd-network/fuzz-dhcp-server.c +++ b/src/libsystemd-network/fuzz-dhcp-server.c @@ -17,36 +17,59 @@ ssize_t sendmsg(int sockfd, const struct msghdr *msg, int flags) { return 0; } -static void add_lease(sd_dhcp_server *server, const struct in_addr *server_address, uint8_t i) { +static int add_lease(sd_dhcp_server *server, const struct in_addr *server_address, uint8_t i) { + _cleanup_(dhcp_lease_freep) DHCPLease *lease = NULL; static const uint8_t chaddr[] = {3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3}; - DHCPLease *lease; + int r; assert(server); - assert_se(lease = new0(DHCPLease, 1)); + lease = new0(DHCPLease, 1); + if (!lease) + return -ENOMEM; + + lease->client_id.data = malloc(2); + if (!lease->client_id.data) + return -ENOMEM; + lease->client_id.length = 2; - assert_se(lease->client_id.data = malloc(2)); lease->client_id.data[0] = 2; lease->client_id.data[1] = i; + lease->address = htobe32(UINT32_C(10) << 24 | i); lease->gateway = server_address->s_addr; lease->expiration = UINT64_MAX; lease->htype = ARPHRD_ETHER; lease->hlen = ETH_ALEN; memcpy(lease->chaddr, chaddr, ETH_ALEN); - assert_se(hashmap_ensure_put(&server->bound_leases_by_client_id, &dhcp_lease_hash_ops, &lease->client_id, lease) >= 0); - assert_se(hashmap_ensure_put(&server->bound_leases_by_address, NULL, UINT32_TO_PTR(lease->address), lease) >= 0); - lease->server = server; + + lease->server = server; /* This must be set just before hashmap_put(). */ + + r = hashmap_ensure_put(&server->bound_leases_by_client_id, &dhcp_lease_hash_ops, &lease->client_id, lease); + if (r < 0) + return r; + + r = hashmap_ensure_put(&server->bound_leases_by_address, NULL, UINT32_TO_PTR(lease->address), lease); + if (r < 0) + return r; + + TAKE_PTR(lease); + + return 0; } -static void add_static_lease(sd_dhcp_server *server, uint8_t i) { +static int add_static_lease(sd_dhcp_server *server, uint8_t i) { uint8_t id[2] = { 2, i }; + int r; assert(server); - assert_se(sd_dhcp_server_set_static_lease(server, - &(struct in_addr) { .s_addr = htobe32(UINT32_C(10) << 24 | i)}, - id, ELEMENTSOF(id)) >= 0); + r = sd_dhcp_server_set_static_lease( + server, + &(struct in_addr) { .s_addr = htobe32(UINT32_C(10) << 24 | i)}, + id, ELEMENTSOF(id)); + + return r; } int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { @@ -66,12 +89,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { assert_se(sd_dhcp_server_configure_pool(server, &address, 24, 0, 0) >= 0); /* add leases to the pool to expose additional code paths */ - add_lease(server, &address, 2); - add_lease(server, &address, 3); + assert_se(add_lease(server, &address, 2) >= 0); + assert_se(add_lease(server, &address, 3) >= 0); /* add static leases */ - add_static_lease(server, 3); - add_static_lease(server, 4); + assert_se(add_static_lease(server, 3) >= 0); + assert_se(add_static_lease(server, 4) >= 0); (void) dhcp_server_handle_message(server, (DHCPMessage*) duped, size); diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c index 196867360f..14ac1cf66f 100644 --- a/src/libsystemd-network/sd-dhcp-server.c +++ b/src/libsystemd-network/sd-dhcp-server.c @@ -27,7 +27,7 @@ #define DHCP_DEFAULT_LEASE_TIME_USEC USEC_PER_HOUR #define DHCP_MAX_LEASE_TIME_USEC (USEC_PER_HOUR*12) -static DHCPLease *dhcp_lease_free(DHCPLease *lease) { +DHCPLease *dhcp_lease_free(DHCPLease *lease) { if (!lease) return NULL; @@ -42,8 +42,6 @@ static DHCPLease *dhcp_lease_free(DHCPLease *lease) { return mfree(lease); } -DEFINE_TRIVIAL_CLEANUP_FUNC(DHCPLease*, dhcp_lease_free); - /* configures the server's address and subnet, and optionally the pool's size and offset into the subnet * the whole pool must fit into the subnet, and may not contain the first (any) nor last (broadcast) address * moreover, the server's own address may be in the pool, and is in that case reserved in order not to