sd-device,udev: refuse invalid devlink and store in normalized form

This is especially for the case that the path contains "..".
Prompted by https://github.com/systemd/systemd/pull/27164#issuecomment-1498863858.

This also makes SYMLINK= gracefully handle paths prefixed with "/dev/",
and manage devlink paths with path_hash_ops.

diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h
index 740c58438cfcf672ff85ce6293a3a767880204e2..b903d1afd6468803f06695802b499b457090a07c 100644 (file)
--- a/src/libsystemd/sd-device/device-private.h
+++ b/src/libsystemd/sd-device/device-private.h
@@ -38,7 +38,7 @@ void device_set_db_persist(sd_device *device);
 void device_set_devlink_priority(sd_device *device, int priority);
 int device_ensure_usec_initialized(sd_device *device, sd_device *device_old);
 int device_add_devlink(sd_device *device, const char *devlink);
-void device_remove_devlink(sd_device *device, const char *devlink);
+int device_remove_devlink(sd_device *device, const char *devlink);
 bool device_has_devlink(sd_device *device, const char *devlink);
 int device_add_property(sd_device *device, const char *property, const char *value);
 int device_add_propertyf(sd_device *device, const char *key, const char *format, ...) _printf_(3, 4);

diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c
index e86ec3e75b1c07ef0e2f8a68f7b99a4dfcbf6499..2a5defca65b48ad455c4e936a6c09fb82d29ee53 100644 (file)
--- a/src/libsystemd/sd-device/sd-device.c
+++ b/src/libsystemd/sd-device/sd-device.c
@@ -1473,34 +1473,58 @@ int device_add_tag(sd_device *device, const char *tag, bool both) {
         return 0;
 }
 
+static char *prefix_dev(const char *p) {
+        assert(p);
+
+        if (path_startswith(p, "/dev/"))
+                return strdup(p);
+
+        return path_join("/dev/", p);
+}
+
 int device_add_devlink(sd_device *device, const char *devlink) {
+        char *p;
         int r;
 
         assert(device);
         assert(devlink);
 
-        r = set_put_strdup(&device->devlinks, devlink);
+        if (!path_is_safe(devlink))
+                return -EINVAL;
+
+        p = prefix_dev(devlink);
+        if (!p)
+                return -ENOMEM;
+
+        path_simplify(p);
+
+        r = set_ensure_consume(&device->devlinks, &path_hash_ops_free, p);
         if (r < 0)
                 return r;
 
         device->devlinks_generation++;
         device->property_devlinks_outdated = true;
 
-        return 0;
+        return r; /* return 1 when newly added, 0 when already exists */
 }
 
-void device_remove_devlink(sd_device *device, const char *devlink) {
-        _cleanup_free_ char *s = NULL;
+int device_remove_devlink(sd_device *device, const char *devlink) {
+        _cleanup_free_ char *p = NULL, *s = NULL;
 
         assert(device);
         assert(devlink);
 
-        s = set_remove(device->devlinks, devlink);
+        p = prefix_dev(devlink);
+        if (!p)
+                return -ENOMEM;
+
+        s = set_remove(device->devlinks, p);
         if (!s)
-                return;
+                return 0; /* does not exist */
 
         device->devlinks_generation++;
         device->property_devlinks_outdated = true;
+        return 1; /* removed */
 }
 
 bool device_has_devlink(sd_device *device, const char *devlink) {

diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c
index 58c5c81574115db037fabd6eb0894e9d757ad84b..68f0a363be45837aaa5478422fd2e8927c0f2fe6 100644 (file)
--- a/src/udev/udev-rules.c
+++ b/src/udev/udev-rules.c
@@ -2574,9 +2574,9 @@ static int udev_rule_apply_token_to_event(
                                         count, token->value);
 
                 for (const char *p = buf;;) {
-                        _cleanup_free_ char *word = NULL, *path = NULL;
+                        _cleanup_free_ char *path = NULL;
 
-                        r = extract_first_word(&p, &word, NULL, EXTRACT_RETAIN_ESCAPE);
+                        r = extract_first_word(&p, &path, NULL, EXTRACT_RETAIN_ESCAPE);
                         if (r == -ENOMEM)
                                 return log_oom();
                         if (r < 0) {
@@ -2586,19 +2586,22 @@ static int udev_rule_apply_token_to_event(
                         if (r == 0)
                                 break;
 
-                        path = path_join("/dev/", word);
-                        if (!path)
-                                return log_oom();
-
                         if (token->op == OP_REMOVE) {
-                                device_remove_devlink(dev, path);
-                                log_event_debug(dev, token, "Dropped SYMLINK '%s'", path);
+                                r = device_remove_devlink(dev, path);
+                                if (r == -ENOMEM)
+                                        return log_oom();
+                                if (r < 0)
+                                        log_event_warning_errno(dev, token, r, "Failed to remove devlink '%s', ignoring: %m", path);
+                                else if (r > 0)
+                                        log_event_debug(dev, token, "Dropped SYMLINK '%s'", path);
                         } else {
                                 r = device_add_devlink(dev, path);
+                                if (r == -ENOMEM)
+                                        return log_oom();
                                 if (r < 0)
-                                        return log_event_error_errno(dev, token, r, "Failed to add devlink '%s': %m", path);
-
-                                log_event_debug(dev, token, "Added SYMLINK '%s'", path);
+                                        log_event_warning_errno(dev, token, r, "Failed to add devlink '%s', ignoring: %m", path);
+                                else if (r > 0)
+                                        log_event_debug(dev, token, "Added SYMLINK '%s'", path);
                         }
                 }
                 break;

diff --git a/test/udev-test.pl b/test/udev-test.pl
index a493485fc17b9b38184e1aea50d0f80eaa1edfe3..9b3641b3d94ffb8e5214dec3faf906188aa07058 100755 (executable)
--- a/test/udev-test.pl
+++ b/test/udev-test.pl
@@ -212,13 +212,37 @@ EOF
                         {
                                 devpath         => "/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda/sda1",
                                 exp_links       => ["boot_disk1", "boot_diskXY1"],
-                                not_exp_links   => ["boot_diskXX1", "hoge"],
+                                not_exp_links   => ["boot_diskXX1"],
                         }],
                 rules           => <<EOF
 SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ATTRS{scsi_level}=="6", ATTRS{rev}=="4.06", ATTRS{type}=="0", ATTRS{queue_depth}=="32", SYMLINK+="boot_diskXX%n"
 SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ATTRS{scsi_level}=="6", ATTRS{rev}=="4.06", ATTRS{type}=="0", ATTRS{queue_depth}=="1", SYMLINK+="boot_diskXY%n"
-SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ATTRS{scsi_level}=="6", ATTRS{rev}=="4.06", ATTRS{type}=="0", SYMLINK+="boot_disk%n", SYMLINK+="hoge"
-SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ATTRS{scsi_level}=="6", ATTRS{rev}=="4.06", ATTRS{type}=="0", SYMLINK-="hoge"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ATTRS{scsi_level}=="6", ATTRS{rev}=="4.06", ATTRS{type}=="0", SYMLINK+="boot_disk%n"
+EOF
+        },
+        {
+                desc            => "SYMLINK tests",
+                devices => [
+                        {
+                                devpath         => "/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda/sda1",
+                                exp_links       => ["link1", "link2/foo", "link3/aaa/bbb",
+                                                    "default___replace_test/foo_aaa",
+                                                    "string_escape___replace/foo_bbb",
+                                                    "env_with_space",
+                                                    "default/replace/mode_foo__hoge",
+                                                    "replace_env_harder_foo__hoge"],
+                                not_exp_links   => ["removed", "unsafe/../../path"],
+                        }],
+                rules           => <<EOF
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", SYMLINK+="removed"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", SYMLINK-="removed"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", SYMLINK+="unsafe/../../path"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", SYMLINK+="link1 link2/foo link3/aaa/bbb"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", SYMLINK+="default?;;replace%%test/foo'aaa"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", OPTIONS="string_escape=replace", SYMLINK+="string_escape   replace/foo%%bbb"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ENV{.HOGE}="env    with    space", SYMLINK+="%E{.HOGE}"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", ENV{.HOGE}="default/replace/mode?foo;;hoge", SYMLINK+="%E{.HOGE}"
+SUBSYSTEMS=="scsi", ATTRS{vendor}=="ATA", ATTRS{model}=="ST910021AS", OPTIONS="string_escape=replace", ENV{.HOGE}="replace/env/harder?foo;;hoge", SYMLINK+="%E{.HOGE}"
 EOF
         },
         {