From 5a3586db9a7e455fc54f9aa19652b48edbae3dd9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 15:57:28 +0900 Subject: [PATCH 1/9] socket-util: split out checking valid character for ifname into ifname_valid_char() --- src/basic/socket-util.c | 27 +++++++++++++++++---------- src/basic/socket-util.h | 1 + 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index f6e751a09f..1c353e86b0 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -748,6 +748,22 @@ static const char* const ip_tos_table[] = { DEFINE_STRING_TABLE_LOOKUP_WITH_FALLBACK(ip_tos, int, 0xff); +bool ifname_valid_char(char a) { + if ((unsigned char) a >= 127U) + return false; + + if ((unsigned char) a <= 32U) + return false; + + if (IN_SET(a, + ':', /* colons are used by the legacy "alias" interface logic */ + '/', /* slashes cannot work, since we need to use network interfaces in sysfs paths, and in paths slashes are separators */ + '%')) /* %d is used in the kernel's weird foo%d format string naming feature which we really really don't want to ever run into by accident */ + return false; + + return true; +} + bool ifname_valid_full(const char *p, IfnameValidFlags flags) { bool numeric = true; @@ -781,16 +797,7 @@ bool ifname_valid_full(const char *p, IfnameValidFlags flags) { return false; for (const char *t = p; *t; t++) { - if ((unsigned char) *t >= 127U) - return false; - - if ((unsigned char) *t <= 32U) - return false; - - if (IN_SET(*t, - ':', /* colons are used by the legacy "alias" interface logic */ - '/', /* slashes cannot work, since we need to use network interfaces in sysfs paths, and in paths slashes are separators */ - '%')) /* %d is used in the kernel's weird foo%d format string naming feature which we really really don't want to ever run into by accident */ + if (!ifname_valid_char(*t)) return false; numeric = numeric && (*t >= '0' && *t <= '9'); diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h index e0b959f5da..f92e425fd6 100644 --- a/src/basic/socket-util.h +++ b/src/basic/socket-util.h @@ -139,6 +139,7 @@ typedef enum { IFNAME_VALID_NUMERIC = 1 << 1, _IFNAME_VALID_ALL = IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC, } IfnameValidFlags; +bool ifname_valid_char(char a); bool ifname_valid_full(const char *p, IfnameValidFlags flags); static inline bool ifname_valid(const char *p) { return ifname_valid_full(p, 0); From e1ecfef16f74dac23803c45f145939027e18eda9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 15:58:31 +0900 Subject: [PATCH 2/9] udev-util: introduce udev_replace_ifname() --- src/shared/udev-util.c | 17 +++++++++++++++++ src/shared/udev-util.h | 1 + 2 files changed, 18 insertions(+) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index 06aede9d36..f934fc157e 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -18,6 +18,7 @@ #include "parse-util.h" #include "path-util.h" #include "signal-util.h" +#include "socket-util.h" #include "string-table.h" #include "string-util.h" #include "strxcpyx.h" @@ -436,6 +437,22 @@ size_t udev_replace_whitespace(const char *str, char *to, size_t len) { return j; } +size_t udev_replace_ifname(char *str) { + size_t replaced = 0; + + assert(str); + + /* See ifname_valid_full(). */ + + for (char *p = str; *p != '\0'; p++) + if (!ifname_valid_char(*p)) { + *p = '_'; + replaced++; + } + + return replaced; +} + size_t udev_replace_chars(char *str, const char *allow) { size_t i = 0, replaced = 0; diff --git a/src/shared/udev-util.h b/src/shared/udev-util.h index d1c33b86a7..276686da80 100644 --- a/src/shared/udev-util.h +++ b/src/shared/udev-util.h @@ -46,6 +46,7 @@ void log_device_uevent(sd_device *device, const char *str); int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos); size_t udev_replace_whitespace(const char *str, char *to, size_t len); +size_t udev_replace_ifname(char *str); size_t udev_replace_chars(char *str, const char *allow); int udev_resolve_subsys_kernel(const char *string, char *result, size_t maxsize, bool read_value); From d37f3e3ec560da810c00610717b73edb380a7a48 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 15:39:04 +0900 Subject: [PATCH 3/9] udev: only network interface can be renamed --- src/udev/udev-rules.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index c37c86a541..d2fc1fcced 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -2053,6 +2053,13 @@ static int udev_rule_apply_token_to_event( if (token->op == OP_ASSIGN_FINAL) event->name_final = true; + if (sd_device_get_ifindex(dev, NULL) < 0) { + log_rule_error(dev, rules, + "Only network interface can be renamed, ignoring NAME=\"%s\"; please fix it.", + token->value); + break; + } + (void) udev_event_apply_format(event, token->value, buf, sizeof(buf), false); if (IN_SET(event->esc, ESCAPE_UNSET, ESCAPE_REPLACE)) { count = udev_replace_chars(buf, "/"); @@ -2060,14 +2067,6 @@ static int udev_rule_apply_token_to_event( log_rule_debug(dev, rules, "Replaced %zu character(s) from result of NAME=\"%s\"", count, token->value); } - if (sd_device_get_devnum(dev, NULL) >= 0 && - (sd_device_get_devname(dev, &val) < 0 || - !streq_ptr(buf, path_startswith(val, "/dev/")))) { - log_rule_error(dev, rules, - "Kernel device nodes cannot be renamed, ignoring NAME=\"%s\"; please fix it.", - token->value); - break; - } r = free_and_strdup_warn(&event->name, buf); if (r < 0) return r; From b4d885f0e861b2d1bb5a62311c61a96f5222b026 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 16:10:26 +0900 Subject: [PATCH 4/9] udev: introduce new netif naming scheme flag to strictly replace ifname --- src/shared/netif-naming-scheme.h | 3 ++- src/udev/udev-rules.c | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shared/netif-naming-scheme.h b/src/shared/netif-naming-scheme.h index c0f94d0407..119b80178f 100644 --- a/src/shared/netif-naming-scheme.h +++ b/src/shared/netif-naming-scheme.h @@ -34,6 +34,7 @@ typedef enum NamingSchemeFlags { NAMING_BRIDGE_NO_SLOT = 1 << 9, /* Don't use PCI hotplug slot information if the corresponding device is a PCI bridge */ NAMING_SLOT_FUNCTION_ID = 1 << 10, /* Use function_id if present to identify PCI hotplug slots */ NAMING_16BIT_INDEX = 1 << 11, /* Allow full 16-bit for the onboard index */ + NAMING_REPLACE_STRICTLY = 1 << 12, /* Use udev_replace_ifname() for NAME= rule */ /* And now the masks that combine the features above */ NAMING_V238 = 0, @@ -43,7 +44,7 @@ typedef enum NamingSchemeFlags { NAMING_V243 = NAMING_V241 | NAMING_NETDEVSIM | NAMING_LABEL_NOPREFIX, NAMING_V245 = NAMING_V243 | NAMING_NSPAWN_LONG_HASH, NAMING_V247 = NAMING_V245 | NAMING_BRIDGE_NO_SLOT, - NAMING_V249 = NAMING_V247 | NAMING_SLOT_FUNCTION_ID | NAMING_16BIT_INDEX, + NAMING_V249 = NAMING_V247 | NAMING_SLOT_FUNCTION_ID | NAMING_16BIT_INDEX | NAMING_REPLACE_STRICTLY, _NAMING_SCHEME_FLAGS_INVALID = -EINVAL, } NamingSchemeFlags; diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index d2fc1fcced..8e5b043658 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -17,6 +17,7 @@ #include "glob-util.h" #include "list.h" #include "mkdir.h" +#include "netif-naming-scheme.h" #include "nulstr-util.h" #include "parse-util.h" #include "path-util.h" @@ -2062,7 +2063,10 @@ static int udev_rule_apply_token_to_event( (void) udev_event_apply_format(event, token->value, buf, sizeof(buf), false); if (IN_SET(event->esc, ESCAPE_UNSET, ESCAPE_REPLACE)) { - count = udev_replace_chars(buf, "/"); + if (naming_scheme_has(NAMING_REPLACE_STRICTLY)) + count = udev_replace_ifname(buf); + else + count = udev_replace_chars(buf, "/"); if (count > 0) log_rule_debug(dev, rules, "Replaced %zu character(s) from result of NAME=\"%s\"", count, token->value); From 068b0f77289411ef9f92f5d701759e98145a06e4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 16:17:21 +0900 Subject: [PATCH 5/9] udev: refuse invalid ifname earlier --- src/udev/udev-event.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 8a01e2512e..b28089be71 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -17,6 +17,7 @@ #include "fd-util.h" #include "fs-util.h" #include "format-util.h" +#include "netif-naming-scheme.h" #include "netlink-util.h" #include "parse-util.h" #include "path-util.h" @@ -848,6 +849,12 @@ static int rename_netif(UdevEvent *event) { if (r < 0) return log_device_error_errno(dev, r, "Failed to get ifindex: %m"); + if (naming_scheme_has(NAMING_REPLACE_STRICTLY) && + !ifname_valid(event->name)) { + log_device_warning(dev, "Invalid network interface name, ignoring: %s", event->name); + return 0; + } + /* Set ID_RENAMING boolean property here, and drop it in the corresponding move uevent later. */ r = device_add_property(dev, "ID_RENAMING", "1"); if (r < 0) From 51c2f543d1474c2615fb8282ea90b2954db33a7e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 16:33:14 +0900 Subject: [PATCH 6/9] udev: fix key name in debug log --- src/udev/udev-rules.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 8e5b043658..d34f9be39e 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -2099,7 +2099,8 @@ static int udev_rule_apply_token_to_event( else count = 0; if (count > 0) - log_rule_debug(dev, rules, "Replaced %zu character(s) from result of LINK", count); + log_rule_debug(dev, rules, "Replaced %zu character(s) from result of SYMLINK=\"%s\"", + count, token->value); p = skip_leading_chars(buf, NULL); while (!isempty(p)) { From ea0f4578a7e90f5227817058bfb11bb91dbb1431 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 14 Apr 2021 15:13:54 +0900 Subject: [PATCH 7/9] udev: replace unsafe characters on assigning ENV{key}="val" when OPTIONS="string_escape=replace" is set Strictly speaking, this breaks backward compatibility, as previously `ENV{key}="val"` ignored `string_escape=` option. But, introducing a new option such as `string_escape=hoge` sounds overkill for me. The default escape mode is `ESCAPE_UNSET`, so I hope this merely break existing rules. --- src/udev/udev-rules.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index d34f9be39e..8713186226 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -2024,6 +2024,12 @@ static int udev_rule_apply_token_to_event( l = strpcpyl(&p, l, val, " ", NULL); (void) udev_event_apply_format(event, token->value, p, l, false); + if (event->esc == ESCAPE_REPLACE) { + count = udev_replace_chars(buf, NULL); + if (count > 0) + log_rule_debug(dev, rules, "Replaced %zu slash(es) from result of ENV{%s}%s=\"%s\"", + count, name, token->op == OP_ADD ? "+" : "", token->value); + } r = device_add_property(dev, name, value_new); if (r < 0) From 91c27ac686261fcca913ac6e3fe1520f38440dcb Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 23 Jun 2021 16:58:20 +0900 Subject: [PATCH 8/9] man: update description of "string_escape=" udev option --- man/udev.xml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/man/udev.xml b/man/udev.xml index 8782bb15c5..f6ea2abc12 100644 --- a/man/udev.xml +++ b/man/udev.xml @@ -117,7 +117,7 @@ := - Assign a value to a key finally; disallow any later changes. + Assign a value to a key finally; disallow any later changes. @@ -607,9 +607,12 @@ - Usually, control and other possibly unsafe characters are replaced - in strings used for device naming. The mode of replacement can be specified - with this option. + When replace, possibly unsafe characters in strings + assigned to NAME, SYMLINK, and + ENV{key} are replaced. When + none, no replacement is performed. When unset, the replacement + is performed for NAME, SYMLINK, but not for + ENV{key}. Defaults to unset. From 5118e8e71dda211d20e34ec8d3012186ba27d3d3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 14 Apr 2021 15:50:36 +0900 Subject: [PATCH 9/9] udev: remove unsafe characters from ID_SERIAL for nvme Fixes #19309. --- rules.d/60-persistent-storage.rules | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules.d/60-persistent-storage.rules b/rules.d/60-persistent-storage.rules index 50b357f8df..b261821214 100644 --- a/rules.d/60-persistent-storage.rules +++ b/rules.d/60-persistent-storage.rules @@ -38,13 +38,13 @@ KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ATTRS{wwid}=="?*", ENV{ID_WWN KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ATTRS{model}=="?*", ENV{ID_MODEL}="$attr{model}" KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ATTRS{firmware_rev}=="?*", ENV{ID_REVISION}="$attr{firmware_rev}" KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \ - ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}" + OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}" KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ATTRS{serial}=="?*", ENV{ID_SERIAL_SHORT}="$attr{serial}" KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ATTRS{model}=="?*", ENV{ID_MODEL}="$attr{model}" KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ATTRS{firmware_rev}=="?*", ENV{ID_REVISION}="$attr{firmware_rev}" KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \ - ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n" + OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n" # virtio-blk KERNEL=="vd*[!0-9]", ATTRS{serial}=="?*", ENV{ID_SERIAL}="$attr{serial}", SYMLINK+="disk/by-id/virtio-$env{ID_SERIAL}"