From c6b3370ab21f66812bec3712b515044218df0b59 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Feb 2019 13:57:31 +0900 Subject: [PATCH 01/13] udev: drop unused Ethernet section --- src/udev/net/link-config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index 5141dae561..b54090046e 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -152,7 +152,7 @@ static int load_link(link_config_ctx *ctx, const char *filename) { link->features[i] = -1; r = config_parse(NULL, filename, file, - "Match\0Link\0Ethernet\0", + "Match\0Link\0", config_item_perf_lookup, link_config_gperf_lookup, CONFIG_PARSE_WARN, link); if (r < 0) From f2d251cdb8f8318666599fc47ee506c59b274ce0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 11:48:39 +0900 Subject: [PATCH 02/13] udev/net: shorten load_link() a little bit --- src/udev/net/link-config.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index b54090046e..b48e423422 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -126,12 +126,8 @@ static int load_link(link_config_ctx *ctx, const char *filename) { assert(filename); file = fopen(filename, "re"); - if (!file) { - if (errno == ENOENT) - return 0; - else - return -errno; - } + if (!file) + return errno == ENOENT ? 0 : -errno; if (null_or_empty_fd(fileno(file))) { log_debug("Skipping empty file: %s", filename); From e8a42907ed8d8206917c5b11d902de444a3aa91d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 11:50:57 +0900 Subject: [PATCH 03/13] udev/net: ignore errors in loading .link files but warn about that --- src/udev/net/link-config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index b48e423422..101d9d262b 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -136,7 +136,7 @@ static int load_link(link_config_ctx *ctx, const char *filename) { link = new0(link_config, 1); if (!link) - return log_oom(); + return -ENOMEM; link->mac_policy = _MACPOLICY_INVALID; link->wol = _WOL_INVALID; @@ -161,7 +161,7 @@ static int load_link(link_config_ctx *ctx, const char *filename) { link->filename = strdup(filename); if (!link->filename) - return log_oom(); + return -ENOMEM; LIST_PREPEND(links, ctx->links, link); link = NULL; @@ -213,7 +213,7 @@ int link_config_load(link_config_ctx *ctx) { STRV_FOREACH_BACKWARDS(f, files) { r = load_link(ctx, *f); if (r < 0) - return r; + log_error_errno(r, "Failed to load %s, ignoring: %m", *f); } return 0; From 79a60834e2212017babdc3a29e6201e495c21cbc Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 11:51:32 +0900 Subject: [PATCH 04/13] udev/net: use size_t for index at one more place --- src/udev/net/link-config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index 101d9d262b..9b422fde43 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -119,7 +119,7 @@ int link_config_ctx_new(link_config_ctx **ret) { static int load_link(link_config_ctx *ctx, const char *filename) { _cleanup_(link_config_freep) link_config *link = NULL; _cleanup_fclose_ FILE *file = NULL; - int i; + size_t i; int r; assert(ctx); @@ -144,7 +144,7 @@ static int load_link(link_config_ctx *ctx, const char *filename) { link->port = _NET_DEV_PORT_INVALID; link->autonegotiation = -1; - for (i = 0; i < (int)ELEMENTSOF(link->features); i++) + for (i = 0; i < ELEMENTSOF(link->features); i++) link->features[i] = -1; r = config_parse(NULL, filename, file, From 6cdab9f17f7a8066abc97519dbb8e15a4d012efe Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 11:53:57 +0900 Subject: [PATCH 05/13] udev/net: use structured initializer at one more place --- src/udev/net/link-config.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index 9b422fde43..7065ec7432 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -119,6 +119,7 @@ int link_config_ctx_new(link_config_ctx **ret) { static int load_link(link_config_ctx *ctx, const char *filename) { _cleanup_(link_config_freep) link_config *link = NULL; _cleanup_fclose_ FILE *file = NULL; + _cleanup_free_ char *name = NULL; size_t i; int r; @@ -134,15 +135,22 @@ static int load_link(link_config_ctx *ctx, const char *filename) { return 0; } - link = new0(link_config, 1); + name = strdup(filename); + if (!name) + return -ENOMEM; + + link = new(link_config, 1); if (!link) return -ENOMEM; - link->mac_policy = _MACPOLICY_INVALID; - link->wol = _WOL_INVALID; - link->duplex = _DUP_INVALID; - link->port = _NET_DEV_PORT_INVALID; - link->autonegotiation = -1; + *link = (link_config) { + .filename = TAKE_PTR(name), + .mac_policy = _MACPOLICY_INVALID, + .wol = _WOL_INVALID, + .duplex = _DUP_INVALID, + .port = _NET_DEV_PORT_INVALID, + .autonegotiation = -1, + }; for (i = 0; i < ELEMENTSOF(link->features); i++) link->features[i] = -1; @@ -153,15 +161,11 @@ static int load_link(link_config_ctx *ctx, const char *filename) { CONFIG_PARSE_WARN, link); if (r < 0) return r; - else - log_debug("Parsed configuration file %s", filename); if (link->speed > UINT_MAX) return -ERANGE; - link->filename = strdup(filename); - if (!link->filename) - return -ENOMEM; + log_debug("Parsed configuration file %s", filename); LIST_PREPEND(links, ctx->links, link); link = NULL; From 176d9c0e11ee9de2ebcfadbaf9c422d3d2e69647 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 11:54:50 +0900 Subject: [PATCH 06/13] udev/net: drop .link files earlier when their conditions do not match system environment --- src/udev/net/link-config.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index 7065ec7432..b25fa1acde 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -165,6 +165,14 @@ static int load_link(link_config_ctx *ctx, const char *filename) { if (link->speed > UINT_MAX) return -ERANGE; + if (!net_match_config(NULL, NULL, NULL, NULL, NULL, + link->match_host, link->match_virt, link->match_kernel_cmdline, + link->match_kernel_version, link->match_arch, + NULL, NULL, NULL, NULL, NULL)) { + log_debug("%s: Conditions do not match the system environment, skipping.", filename); + return 0; + } + log_debug("Parsed configuration file %s", filename); LIST_PREPEND(links, ctx->links, link); From 391f6bc1dbf7f0171e556dc23d0770dfecfbbbde Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Feb 2019 15:16:47 +0900 Subject: [PATCH 07/13] udev: fix memleak in conditions for .link file --- src/udev/net/link-config.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index b25fa1acde..aa3e7eeae3 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -50,12 +50,13 @@ static void link_config_free(link_config *link) { strv_free(link->match_path); strv_free(link->match_driver); strv_free(link->match_type); - free(link->match_name); - free(link->match_host); - free(link->match_virt); - free(link->match_kernel_cmdline); - free(link->match_kernel_version); - free(link->match_arch); + strv_free(link->match_name); + + condition_free_list(link->match_host); + condition_free_list(link->match_virt); + condition_free_list(link->match_kernel_cmdline); + condition_free_list(link->match_kernel_version); + condition_free_list(link->match_arch); free(link->description); free(link->mac); From 84fb56d396b14fb778dbe51f02d247f0899a74ef Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 12:35:40 +0900 Subject: [PATCH 08/13] udev/ethtool: fix error detection of ethtool_link_mode_bit_from_string() --- src/udev/net/ethtool-util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/udev/net/ethtool-util.c b/src/udev/net/ethtool-util.c index 6a0a42386b..55620926dc 100644 --- a/src/udev/net/ethtool-util.c +++ b/src/udev/net/ethtool-util.c @@ -775,7 +775,9 @@ int config_parse_advertise(const char *unit, break; mode = ethtool_link_mode_bit_from_string(w); - if (mode < 0) { + /* We reuse the kernel provided enum which does not contain negative value. So, the cast + * below is mandatory. Otherwise, the check below always passes and access an invalid address. */ + if ((int) mode < 0) { log_syntax(unit, LOG_ERR, filename, line, 0, "Failed to parse advertise mode, ignoring: %s", w); continue; } From 7cd1f60df0608597e954508b371b42df37d2e9c8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 25 Feb 2019 12:02:17 +0900 Subject: [PATCH 09/13] fuzz: use fflush() and drop unnecessary rewind() --- src/network/fuzz-netdev-parser.c | 2 +- src/network/fuzz-network-parser.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/fuzz-netdev-parser.c b/src/network/fuzz-netdev-parser.c index 37f7cf8ccd..78c8d0c2b5 100644 --- a/src/network/fuzz-netdev-parser.c +++ b/src/network/fuzz-netdev-parser.c @@ -18,7 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size != 0) assert_se(fwrite(data, size, 1, f) == 1); - rewind(f); + fflush(f); assert_se(manager_new(&manager) >= 0); (void) netdev_load_one(manager, netdev_config); return 0; diff --git a/src/network/fuzz-network-parser.c b/src/network/fuzz-network-parser.c index 4b79500805..e4b789c167 100644 --- a/src/network/fuzz-network-parser.c +++ b/src/network/fuzz-network-parser.c @@ -18,7 +18,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size != 0) assert_se(fwrite(data, size, 1, f) == 1); - rewind(f); + fflush(f); assert_se(manager_new(&manager) >= 0); (void) network_load_one(manager, network_config); return 0; From a378400b3fac41b6d41f5ee52057c397ad82a55a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Feb 2019 14:28:08 +0900 Subject: [PATCH 10/13] fuzz: add fuzzer for parsing .link files This also renames load_link() to link_load_one() --- src/udev/meson.build | 11 +++++++++++ src/udev/net/fuzz-link-parser.c | 25 +++++++++++++++++++++++++ src/udev/net/link-config.c | 6 ++---- src/udev/net/link-config.h | 2 ++ 4 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 src/udev/net/fuzz-link-parser.c diff --git a/src/udev/meson.build b/src/udev/meson.build index 9d3f6d1c56..2de88c0d93 100644 --- a/src/udev/meson.build +++ b/src/udev/meson.build @@ -197,3 +197,14 @@ configure_file( meson.add_install_script('sh', '-c', mkdir_p.format(join_paths(sysconfdir, 'udev/rules.d'))) + +fuzzers += [ + [['src/udev/net/fuzz-link-parser.c', + 'src/fuzz/fuzz.h'], + [libudev_core, + libudev_static, + libsystemd_network, + libshared], + [threads, + libacl]] + ] diff --git a/src/udev/net/fuzz-link-parser.c b/src/udev/net/fuzz-link-parser.c new file mode 100644 index 0000000000..397fa2c16e --- /dev/null +++ b/src/udev/net/fuzz-link-parser.c @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ + +#include "fd-util.h" +#include "fs-util.h" +#include "fuzz.h" +#include "link-config.h" +#include "tmpfile-util.h" + +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { + _cleanup_(link_config_ctx_freep) link_config_ctx *ctx = NULL; + _cleanup_(unlink_tempfilep) char filename[] = "/tmp/fuzz-link-config.XXXXXX"; + _cleanup_fclose_ FILE *f = NULL; + + if (!getenv("SYSTEMD_LOG_LEVEL")) + log_set_max_level(LOG_CRIT); + + assert_se(fmkostemp_safe(filename, "r+", &f) == 0); + if (size != 0) + assert_se(fwrite(data, size, 1, f) == 1); + + fflush(f); + assert_se(link_config_ctx_new(&ctx) >= 0); + (void) link_load_one(ctx, filename); + return 0; +} diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index aa3e7eeae3..f1d36ccac0 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -94,8 +94,6 @@ void link_config_ctx_free(link_config_ctx *ctx) { return; } -DEFINE_TRIVIAL_CLEANUP_FUNC(link_config_ctx*, link_config_ctx_free); - int link_config_ctx_new(link_config_ctx **ret) { _cleanup_(link_config_ctx_freep) link_config_ctx *ctx = NULL; @@ -117,7 +115,7 @@ int link_config_ctx_new(link_config_ctx **ret) { return 0; } -static int load_link(link_config_ctx *ctx, const char *filename) { +int link_load_one(link_config_ctx *ctx, const char *filename) { _cleanup_(link_config_freep) link_config *link = NULL; _cleanup_fclose_ FILE *file = NULL; _cleanup_free_ char *name = NULL; @@ -224,7 +222,7 @@ int link_config_load(link_config_ctx *ctx) { return log_error_errno(r, "failed to enumerate link files: %m"); STRV_FOREACH_BACKWARDS(f, files) { - r = load_link(ctx, *f); + r = link_load_one(ctx, *f); if (r < 0) log_error_errno(r, "Failed to load %s, ignoring: %m", *f); } diff --git a/src/udev/net/link-config.h b/src/udev/net/link-config.h index 1113b1052e..4335b593d1 100644 --- a/src/udev/net/link-config.h +++ b/src/udev/net/link-config.h @@ -67,7 +67,9 @@ struct link_config { int link_config_ctx_new(link_config_ctx **ret); void link_config_ctx_free(link_config_ctx *ctx); +DEFINE_TRIVIAL_CLEANUP_FUNC(link_config_ctx*, link_config_ctx_free); +int link_load_one(link_config_ctx *ctx, const char *filename); int link_config_load(link_config_ctx *ctx); bool link_config_should_reload(link_config_ctx *ctx); From eac31d708f83aa716c09f1ba9e31be9beafe979e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Feb 2019 14:45:46 +0900 Subject: [PATCH 11/13] fuzz: add directives.link and 99-default.link for fuzz-link-parser Also adds several reproducers of errors fixed in earlier commits. --- test/fuzz/fuzz-link-parser/99-default.link | 12 +++++++ .../fuzz/fuzz-link-parser/advertise-segv.link | 2 ++ .../fuzz-link-parser/condition-memleak.link | 7 ++++ test/fuzz/fuzz-link-parser/directives.link | 35 +++++++++++++++++++ 4 files changed, 56 insertions(+) create mode 100644 test/fuzz/fuzz-link-parser/99-default.link create mode 100644 test/fuzz/fuzz-link-parser/advertise-segv.link create mode 100644 test/fuzz/fuzz-link-parser/condition-memleak.link create mode 100644 test/fuzz/fuzz-link-parser/directives.link diff --git a/test/fuzz/fuzz-link-parser/99-default.link b/test/fuzz/fuzz-link-parser/99-default.link new file mode 100644 index 0000000000..92fcbe83ea --- /dev/null +++ b/test/fuzz/fuzz-link-parser/99-default.link @@ -0,0 +1,12 @@ +# SPDX-License-Identifier: LGPL-2.1+ +# +# This file is part of systemd. +# +# systemd is free software; you can redistribute it and/or modify it +# under the terms of the GNU Lesser General Public License as published by +# the Free Software Foundation; either version 2.1 of the License, or +# (at your option) any later version. + +[Link] +NamePolicy=keep kernel database onboard slot path +MACAddressPolicy=persistent diff --git a/test/fuzz/fuzz-link-parser/advertise-segv.link b/test/fuzz/fuzz-link-parser/advertise-segv.link new file mode 100644 index 0000000000..305e846a49 --- /dev/null +++ b/test/fuzz/fuzz-link-parser/advertise-segv.link @@ -0,0 +1,2 @@ +[Link] +Advertise=hoge foo diff --git a/test/fuzz/fuzz-link-parser/condition-memleak.link b/test/fuzz/fuzz-link-parser/condition-memleak.link new file mode 100644 index 0000000000..523e4ba6cb --- /dev/null +++ b/test/fuzz/fuzz-link-parser/condition-memleak.link @@ -0,0 +1,7 @@ +[Match] +OriginalName=eth0 +Host=hogehoge +Virtualization=qemu +KernelCommandLine=aaa +KernelVersion=4.20.3 +Architecture=x86-64 diff --git a/test/fuzz/fuzz-link-parser/directives.link b/test/fuzz/fuzz-link-parser/directives.link new file mode 100644 index 0000000000..5925e5ad12 --- /dev/null +++ b/test/fuzz/fuzz-link-parser/directives.link @@ -0,0 +1,35 @@ +[Match] +MACAddress= +OriginalName= +Path= +Driver= +Type= +Host= +Virtualization= +KernelCommandLine= +KernelVersion= +Architecture= +[Link] +Description= +MACAddressPolicy= +MACAddress= +NamePolicy= +Name= +Alias= +MTUBytes= +BitsPerSecond= +Duplex= +AutoNegotiation= +WakeOnLan= +Port= +GenericSegmentationOffload= +TCPSegmentationOffload= +TCP6SegmentationOffload= +UDPSegmentationOffload= +GenericReceiveOffload= +LargeReceiveOffload= +RxChannels= +TxChannels= +OtherChannels= +CombinedChannels= +Advertise= From 810d43a8e86aacce6c50e196e117ddb7c3f71aa8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Feb 2019 14:47:36 +0900 Subject: [PATCH 12/13] tools: update check-directives.sh to support fuzz-link-parser --- tools/check-directives.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/check-directives.sh b/tools/check-directives.sh index e2fd38898f..1c20947b4b 100755 --- a/tools/check-directives.sh +++ b/tools/check-directives.sh @@ -19,3 +19,8 @@ if [[ $(generate_directives src/network/netdev/netdev-gperf.gperf | wc -l) -ne $ echo "Looks like test/fuzz/fuzz-netdev-parser/directives.netdev hasn't been updated" exit 1 fi + +if [[ $(generate_directives src/udev/net/link-config-gperf.gperf | wc -l) -ne $(wc -l Date: Fri, 22 Feb 2019 18:00:54 +0900 Subject: [PATCH 13/13] tools: check all directives even if it detects non-updated files --- tools/check-directives.sh | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/check-directives.sh b/tools/check-directives.sh index 1c20947b4b..99f8f9d451 100755 --- a/tools/check-directives.sh +++ b/tools/check-directives.sh @@ -10,17 +10,20 @@ function generate_directives() { }}' "$1" } +ret=0 if [[ $(generate_directives src/network/networkd-network-gperf.gperf | wc -l) -ne $(wc -l