From b358833ee91551d5d70a4fd754187a0259e2c62f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 22 Dec 2024 01:48:37 +0900 Subject: [PATCH 1/3] udev-config: split on_ctrl_msg() into small pieces No functional change, just refactroing and preparation for later commits. --- src/udev/udev-config.c | 78 ++++++++++++++++++++++++++++++++++++++++- src/udev/udev-config.h | 5 ++- src/udev/udev-manager.c | 59 ++++--------------------------- src/udev/udev-manager.h | 4 +++ 4 files changed, 91 insertions(+), 55 deletions(-) diff --git a/src/udev/udev-config.c b/src/udev/udev-config.c index 891cf925358..d511691ab2b 100644 --- a/src/udev/udev-config.c +++ b/src/udev/udev-config.c @@ -271,7 +271,7 @@ static void manager_merge_config(Manager *manager) { MERGE_BOOL(blockdev_read_only); } -void udev_config_set_default_children_max(UdevConfig *config) { +static void udev_config_set_default_children_max(UdevConfig *config) { uint64_t cpu_limit, mem_limit, cpu_count = 1; int r; @@ -293,6 +293,30 @@ void udev_config_set_default_children_max(UdevConfig *config) { log_debug("Set children_max to %u", config->children_max); } +void manager_set_children_max(Manager *manager, unsigned n) { + assert(manager); + + manager->config_by_control.children_max = n; + /* When 0 is specified, determine the maximum based on the system resources. */ + udev_config_set_default_children_max(&manager->config_by_control); + manager->config.children_max = manager->config_by_control.children_max; + + notify_ready(manager); +} + +void manager_set_log_level(Manager *manager, int log_level) { + assert(manager); + assert(LOG_PRI(log_level) == log_level); + + int old = log_get_max_level(); + + log_set_max_level(log_level); + manager->config.log_level = manager->config_by_control.log_level = log_level; + + if (log_level != old) + manager_kill_workers(manager, /* force = */ false); +} + static void manager_adjust_config(UdevConfig *config) { assert(config); @@ -315,6 +339,58 @@ static void manager_adjust_config(UdevConfig *config) { udev_config_set_default_children_max(config); } +static int manager_set_environment_one(Manager *manager, const char *s) { + int r; + + assert(manager); + assert(s); + + _cleanup_free_ char *key = NULL, *value = NULL; + r = split_pair(s, "=", &key, &value); + if (r < 0) + return r; + + if (isempty(value)) { + _cleanup_free_ char *old_key = NULL, *old_value = NULL; + old_value = hashmap_remove2(manager->properties, key, (void**) &old_key); + return !!old_value; + } + + if (streq_ptr(value, hashmap_get(manager->properties, key))) + return 0; + + _cleanup_free_ char *old_key = NULL, *old_value = NULL; + old_value = hashmap_get2(manager->properties, key, (void**) &old_key); + + r = hashmap_ensure_replace(&manager->properties, &string_hash_ops, key, value); + if (r < 0) { + assert(!old_key); + assert(!old_value); + return r; + } + + TAKE_PTR(key); + TAKE_PTR(value); + return 1; +} + +void manager_set_environment(Manager *manager, char * const *v) { + bool changed = false; + int r; + + assert(manager); + + STRV_FOREACH(s, v) { + r = manager_set_environment_one(manager, *s); + if (r < 0) + log_debug_errno(r, "Failed to update environment '%s', ignoring: %m", *s); + changed = changed || r > 0; + } + + if (changed) + manager_kill_workers(manager, /* force = */ false); +} + int manager_load(Manager *manager, int argc, char *argv[]) { int r; diff --git a/src/udev/udev-config.h b/src/udev/udev-config.h index 1c7a74b1067..68bb1ea98cf 100644 --- a/src/udev/udev-config.h +++ b/src/udev/udev-config.h @@ -26,6 +26,9 @@ typedef struct UdevConfig { .resolve_name_timing = _RESOLVE_NAME_TIMING_INVALID, \ } +void manager_set_children_max(Manager *manager, unsigned n); +void manager_set_log_level(Manager *manager, int log_level); +void manager_set_environment(Manager *manager, char * const *v); + int manager_load(Manager *manager, int argc, char *argv[]); UdevReloadFlags manager_reload_config(Manager *manager); -void udev_config_set_default_children_max(UdevConfig *c); diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index b0f542a11a6..b7bce977d1d 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -194,7 +194,7 @@ static int worker_new(Worker **ret, Manager *manager, sd_device_monitor *worker_ return 0; } -static void manager_kill_workers(Manager *manager, bool force) { +void manager_kill_workers(Manager *manager, bool force) { Worker *worker; assert(manager); @@ -233,7 +233,7 @@ static void manager_exit(Manager *manager) { manager_kill_workers(manager, true); } -static void notify_ready(Manager *manager) { +void notify_ready(Manager *manager) { int r; assert(manager); @@ -844,7 +844,6 @@ static int on_worker(sd_event_source *s, int fd, uint32_t revents, void *userdat /* receive the udevd message from userspace */ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrlMessageValue *value, void *userdata) { Manager *manager = ASSERT_PTR(userdata); - int r; assert(value); @@ -857,13 +856,7 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl log_debug("Received udev control message (SET_LOG_LEVEL), setting log_level=%i", value->intval); - r = log_get_max_level(); - if (r == value->intval) - break; - - log_set_max_level(value->intval); - manager->config.log_level = manager->config_by_control.log_level = value->intval; - manager_kill_workers(manager, false); + manager_set_log_level(manager, value->intval); break; case UDEV_CTRL_STOP_EXEC_QUEUE: log_debug("Received udev control message (STOP_EXEC_QUEUE)"); @@ -879,8 +872,6 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl manager_reload(manager, /* force = */ true); break; case UDEV_CTRL_SET_ENV: { - _unused_ _cleanup_free_ char *old_val = NULL, *old_key = NULL; - _cleanup_free_ char *key = NULL, *val = NULL; const char *eq; eq = strchr(value->buf, '='); @@ -889,41 +880,8 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl return 1; } - key = strndup(value->buf, eq - value->buf); - if (!key) { - log_oom(); - return 1; - } - - old_val = hashmap_remove2(manager->properties, key, (void **) &old_key); - - r = hashmap_ensure_allocated(&manager->properties, &string_hash_ops); - if (r < 0) { - log_oom(); - return 1; - } - - eq++; - if (isempty(eq)) - log_debug("Received udev control message (ENV), unsetting '%s'", key); - else { - val = strdup(eq); - if (!val) { - log_oom(); - return 1; - } - - log_debug("Received udev control message (ENV), setting '%s=%s'", key, val); - - r = hashmap_put(manager->properties, key, val); - if (r < 0) { - log_oom(); - return 1; - } - } - - key = val = NULL; - manager_kill_workers(manager, false); + log_debug("Received udev control message(SET_ENV, %s)", value->buf); + manager_set_environment(manager, STRV_MAKE(value->buf)); break; } case UDEV_CTRL_SET_CHILDREN_MAX: @@ -933,13 +891,8 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl } log_debug("Received udev control message (SET_MAX_CHILDREN), setting children_max=%i", value->intval); - manager->config_by_control.children_max = value->intval; - /* When 0 is specified, determine the maximum based on the system resources. */ - udev_config_set_default_children_max(&manager->config_by_control); - manager->config.children_max = manager->config_by_control.children_max; - - notify_ready(manager); + manager_set_children_max(manager, value->intval); break; case UDEV_CTRL_PING: log_debug("Received udev control message (PING)"); diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index 4f0294a94f3..2690d59bf39 100644 --- a/src/udev/udev-manager.h +++ b/src/udev/udev-manager.h @@ -55,4 +55,8 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free); int manager_init(Manager *manager); int manager_main(Manager *manager); +void notify_ready(Manager *manager); + +void manager_kill_workers(Manager *manager, bool force); + bool devpath_conflict(const char *a, const char *b); From 4c547d216aad4a61d58a6962d0256da3c6ae6a71 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 Jan 2025 21:07:41 +0900 Subject: [PATCH 2/3] udev: introduce udev_property_name_is_valid() and friends --- src/udev/net/link-config.c | 7 ++----- src/udev/udev-def.h | 8 ++++++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index 7d4e334503d..5a4b7261bbd 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -15,7 +15,6 @@ #include "creds-util.h" #include "device-private.h" #include "device-util.h" -#include "env-util.h" #include "escape.h" #include "ethtool-util.h" #include "fd-util.h" @@ -1111,8 +1110,7 @@ int config_parse_udev_property( continue; } - /* The restriction for udev property is not clear. Let's apply the one for environment variable here. */ - if (!env_assignment_is_valid(resolved)) { + if (!udev_property_assignment_is_valid(resolved)) { log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid udev property, ignoring assignment: %s", word); continue; @@ -1181,8 +1179,7 @@ int config_parse_udev_property_name( continue; } - /* The restriction for udev property is not clear. Let's apply the one for environment variable here. */ - if (!env_name_is_valid(resolved)) { + if (!udev_property_name_is_valid(resolved)) { log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid udev property name, ignoring assignment: %s", resolved); continue; diff --git a/src/udev/udev-def.h b/src/udev/udev-def.h index 9d9fc78247d..ed231764bc8 100644 --- a/src/udev/udev-def.h +++ b/src/udev/udev-def.h @@ -3,6 +3,8 @@ #include +#include "env-util.h" + #define UDEV_NAME_SIZE 512 #define UDEV_PATH_SIZE 1024 #define UDEV_LINE_SIZE 16384 @@ -78,3 +80,9 @@ typedef enum UdevReloadFlags { UDEV_RELOAD_KILL_WORKERS = 1u << (_UDEV_BUILTIN_MAX + 0), UDEV_RELOAD_RULES = 1u << (_UDEV_BUILTIN_MAX + 1), } UdevReloadFlags; + +/* udev properties are conceptually close to environment variables. Let's validate names, values, and + * assignments in the same way. */ +#define udev_property_name_is_valid(x) env_name_is_valid(x) +#define udev_property_value_is_valid(x) env_value_is_valid(x) +#define udev_property_assignment_is_valid(x) env_assignment_is_valid(x) From bbb0dbe6b1fad7c8d8250f5dff334a2de8766559 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 22 Dec 2024 06:34:33 +0900 Subject: [PATCH 3/3] udev-ctrl: refuse ENV control message with invalid environment assignment Previously, udevd accepts an arbitrary pair of key and value. Let's make the environment variable assignment more strict for safety. Note, we already refuse environment variables with the same way in net/link-config.c. --- src/udev/udev-manager.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index b7bce977d1d..1768da5a388 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -871,19 +871,15 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl log_debug("Received udev control message (RELOAD)"); manager_reload(manager, /* force = */ true); break; - case UDEV_CTRL_SET_ENV: { - const char *eq; - - eq = strchr(value->buf, '='); - if (!eq) { - log_error("Invalid key format '%s'", value->buf); - return 1; + case UDEV_CTRL_SET_ENV: + if (!udev_property_assignment_is_valid(value->buf)) { + log_debug("Received invalid udev control message(SET_ENV, %s), ignoring.", value->buf); + break; } log_debug("Received udev control message(SET_ENV, %s)", value->buf); manager_set_environment(manager, STRV_MAKE(value->buf)); break; - } case UDEV_CTRL_SET_CHILDREN_MAX: if (value->intval < 0) { log_debug("Received invalid udev control message (SET_MAX_CHILDREN, %i), ignoring.", value->intval);