From ff58f2ae2a981bf106ed438887f989b1edd08174 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 13:07:38 +0900 Subject: [PATCH 01/10] sd-device: verify new syspath on renaming --- src/libsystemd/sd-device/device-private.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index de12ad7e00a..90dcd3a857f 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -747,20 +747,28 @@ int device_new_from_watch_handle_at(sd_device **ret, int dirfd, int wd) { } int device_rename(sd_device *device, const char *name) { - _cleanup_free_ char *dirname = NULL; - const char *new_syspath, *interface; + _cleanup_free_ char *new_syspath = NULL; + const char *interface; int r; assert(device); assert(name); - dirname = dirname_malloc(device->syspath); - if (!dirname) + if (!filename_is_valid(name)) + return -EINVAL; + + r = path_extract_directory(device->syspath, &new_syspath); + if (r < 0) + return r; + + if (!path_extend(&new_syspath, name)) return -ENOMEM; - new_syspath = prefix_roota(dirname, name); + if (!path_is_safe(new_syspath)) + return -EINVAL; - /* the user must trust that the new name is correct */ + /* At the time this is called, the renamed device may not exist yet. Hence, we cannot validate + * the new syspath. */ r = device_set_syspath(device, new_syspath, false); if (r < 0) return r; From f5a75f2027e53bdaf4deb7087fac73f8be6bf4f4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 13:11:08 +0900 Subject: [PATCH 02/10] sd-device: reduce indentation --- src/libsystemd/sd-device/device-private.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 90dcd3a857f..9bf39ecfab2 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -774,19 +774,17 @@ int device_rename(sd_device *device, const char *name) { return r; r = sd_device_get_property_value(device, "INTERFACE", &interface); - if (r >= 0) { - /* like DEVPATH_OLD, INTERFACE_OLD is not saved to the db, but only stays around for the current event */ - r = device_add_property_internal(device, "INTERFACE_OLD", interface); - if (r < 0) - return r; - - r = device_add_property_internal(device, "INTERFACE", name); - if (r < 0) - return r; - } else if (r != -ENOENT) + if (r == -ENOENT) + return 0; + if (r < 0) return r; - return 0; + /* like DEVPATH_OLD, INTERFACE_OLD is not saved to the db, but only stays around for the current event */ + r = device_add_property_internal(device, "INTERFACE_OLD", interface); + if (r < 0) + return r; + + return device_add_property_internal(device, "INTERFACE", name); } int device_shallow_clone(sd_device *old_device, sd_device **new_device) { From 60e50fb20d20acf59cf98e4cc2388aa9aed61014 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 13:38:39 +0900 Subject: [PATCH 03/10] sd-device: reset sysname and sysnum on renaming --- src/libsystemd/sd-device/device-private.c | 4 ++++ src/udev/udev-event.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 9bf39ecfab2..b6043848eae 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -773,6 +773,10 @@ int device_rename(sd_device *device, const char *name) { if (r < 0) return r; + /* Here, only clear the sysname and sysnum. They will be set when requested. */ + device->sysnum = NULL; + device->sysname = mfree(device->sysname); + r = sd_device_get_property_value(device, "INTERFACE", &interface); if (r == -ENOENT) return 0; diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 78e8f3018c5..9b531051999 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -869,15 +869,21 @@ int udev_event_spawn( } static int rename_netif(UdevEvent *event) { - sd_device *dev = event->dev; const char *oldname; + sd_device *dev; unsigned flags; int ifindex, r; + assert(event); + if (!event->name) return 0; /* No new name is requested. */ - r = sd_device_get_sysname(dev, &oldname); + dev = ASSERT_PTR(event->dev); + + /* Read sysname from cloned sd-device object, otherwise use-after-free is triggered, as the + * main object will be renamed and dev->sysname will be freed in device_rename(). */ + r = sd_device_get_sysname(event->dev_db_clone, &oldname); if (r < 0) return log_device_error_errno(dev, r, "Failed to get sysname: %m"); From ce1d08ba949531250d006d28e88202417b11fc0c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 14:25:27 +0900 Subject: [PATCH 04/10] sd-device: use path_extract_filename() at one more place This also does several cleanups. --- src/libsystemd/sd-device/sd-device.c | 35 +++++++++++----------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 6c8e7d646fb..6c2d1243ac3 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1437,37 +1437,30 @@ int device_get_device_id(sd_device *device, const char **ret) { return r; if (sd_device_get_devnum(device, &devnum) >= 0) { - assert(subsystem); - /* use dev_t — b259:131072, c254:0 */ - r = asprintf(&id, "%c%u:%u", + if (asprintf(&id, "%c%u:%u", streq(subsystem, "block") ? 'b' : 'c', - major(devnum), minor(devnum)); - if (r < 0) + major(devnum), minor(devnum)) < 0) return -ENOMEM; } else if (sd_device_get_ifindex(device, &ifindex) >= 0) { /* use netdev ifindex — n3 */ - r = asprintf(&id, "n%u", (unsigned) ifindex); - if (r < 0) + if (asprintf(&id, "n%u", (unsigned) ifindex) < 0) return -ENOMEM; } else { + _cleanup_free_ char *sysname = NULL; + /* use $subsys:$sysname — pci:0000:00:1f.2 - * sysname() has '!' translated, get it from devpath - */ - const char *sysname; + * sd_device_get_sysname() has '!' translated, get it from devpath */ + r = path_extract_filename(device->devpath, &sysname); + if (r < 0) + return r; - sysname = basename(device->devpath); - if (!sysname) - return -EINVAL; - - if (!subsystem) - return -EINVAL; - - if (streq(subsystem, "drivers")) - /* the 'drivers' pseudo-subsystem is special, and needs the real subsystem - * encoded as well */ + if (streq(subsystem, "drivers")) { + /* the 'drivers' pseudo-subsystem is special, and needs the real + * subsystem encoded as well */ + assert(device->driver_subsystem); id = strjoin("+drivers:", device->driver_subsystem, ":", sysname); - else + } else id = strjoin("+", subsystem, ":", sysname); if (!id) return -ENOMEM; From d37c69c1bf3ffd59e2e7e615760e4be678ba4fb1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 14:29:07 +0900 Subject: [PATCH 05/10] sd-device: shorten code a bit --- src/libsystemd/sd-device/device-private.c | 26 ++++++++--------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index b6043848eae..6fa70a8fd39 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -933,15 +933,11 @@ static int device_tag(sd_device *device, const char *tag, bool add) { path = strjoina("/run/udev/tags/", tag, "/", id); - if (add) { - r = touch_file(path, true, USEC_INFINITY, UID_INVALID, GID_INVALID, 0444); - if (r < 0) - return r; - } else { - r = unlink(path); - if (r < 0 && errno != ENOENT) - return -errno; - } + if (add) + return touch_file(path, true, USEC_INFINITY, UID_INVALID, GID_INVALID, 0444); + + if (unlink(path) < 0 && errno != ENOENT) + return -errno; return 0; } @@ -950,16 +946,14 @@ int device_tag_index(sd_device *device, sd_device *device_old, bool add) { const char *tag; int r = 0, k; - if (add && device_old) { + if (add && device_old) /* delete possible left-over tags */ - FOREACH_DEVICE_TAG(device_old, tag) { + FOREACH_DEVICE_TAG(device_old, tag) if (!sd_device_has_tag(device, tag)) { k = device_tag(device_old, tag, false); if (r >= 0 && k < 0) r = k; } - } - } FOREACH_DEVICE_TAG(device, tag) { k = device_tag(device, tag, add); @@ -1017,8 +1011,7 @@ int device_update_db(sd_device *device) { /* do not store anything for otherwise empty devices */ if (!has_info && major(device->devnum) == 0 && device->ifindex == 0) { - r = unlink(path); - if (r < 0 && errno != ENOENT) + if (unlink(path) < 0 && errno != ENOENT) return -errno; return 0; @@ -1106,8 +1099,7 @@ int device_delete_db(sd_device *device) { path = strjoina("/run/udev/data/", id); - r = unlink(path); - if (r < 0 && errno != ENOENT) + if (unlink(path) < 0 && errno != ENOENT) return -errno; return 0; From c77c1cc20127bb29444309c17c18cc5b0bbe04da Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 14:45:45 +0900 Subject: [PATCH 06/10] sd-device: use correct type and parser for device node uid and gid --- src/libsystemd/sd-device/device-private.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 6fa70a8fd39..0bdcbc32658 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -148,13 +148,13 @@ int device_get_devnode_uid(sd_device *device, uid_t *uid) { } static int device_set_devuid(sd_device *device, const char *uid) { - unsigned u; + uid_t u; int r; assert(device); assert(uid); - r = safe_atou(uid, &u); + r = parse_uid(uid, &u); if (r < 0) return r; @@ -186,13 +186,13 @@ int device_get_devnode_gid(sd_device *device, gid_t *gid) { } static int device_set_devgid(sd_device *device, const char *gid) { - unsigned g; + gid_t g; int r; assert(device); assert(gid); - r = safe_atou(gid, &g); + r = parse_gid(gid, &g); if (r < 0) return r; From d82827a107d7568c09a4997617495ff544fd1eec Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 14:51:13 +0900 Subject: [PATCH 07/10] sd-device: rename function arguments for storing results --- src/libsystemd/sd-device/device-private.c | 24 +++++++++++------------ src/libsystemd/sd-device/device-private.h | 8 ++++---- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 0bdcbc32658..6ad49c05a07 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -111,7 +111,7 @@ uint64_t device_get_devlinks_generation(sd_device *device) { return device->devlinks_generation; } -int device_get_devnode_mode(sd_device *device, mode_t *mode) { +int device_get_devnode_mode(sd_device *device, mode_t *ret) { int r; assert(device); @@ -123,13 +123,13 @@ int device_get_devnode_mode(sd_device *device, mode_t *mode) { if (device->devmode == MODE_INVALID) return -ENOENT; - if (mode) - *mode = device->devmode; + if (ret) + *ret = device->devmode; return 0; } -int device_get_devnode_uid(sd_device *device, uid_t *uid) { +int device_get_devnode_uid(sd_device *device, uid_t *ret) { int r; assert(device); @@ -141,8 +141,8 @@ int device_get_devnode_uid(sd_device *device, uid_t *uid) { if (device->devuid == UID_INVALID) return -ENOENT; - if (uid) - *uid = device->devuid; + if (ret) + *ret = device->devuid; return 0; } @@ -167,7 +167,7 @@ static int device_set_devuid(sd_device *device, const char *uid) { return 0; } -int device_get_devnode_gid(sd_device *device, gid_t *gid) { +int device_get_devnode_gid(sd_device *device, gid_t *ret) { int r; assert(device); @@ -179,8 +179,8 @@ int device_get_devnode_gid(sd_device *device, gid_t *gid) { if (device->devgid == GID_INVALID) return -ENOENT; - if (gid) - *gid = device->devgid; + if (ret) + *ret = device->devgid; return 0; } @@ -593,17 +593,17 @@ int device_get_properties_strv(sd_device *device, char ***strv) { return 0; } -int device_get_devlink_priority(sd_device *device, int *priority) { +int device_get_devlink_priority(sd_device *device, int *ret) { int r; assert(device); - assert(priority); r = device_read_db(device); if (r < 0) return r; - *priority = device->devlink_priority; + if (ret) + *ret = device->devlink_priority; return 0; } diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 68caee2cb46..9b2557a39c7 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -18,11 +18,11 @@ static inline int device_new_from_watch_handle(sd_device **ret, int wd) { } int device_get_device_id(sd_device *device, const char **ret); -int device_get_devlink_priority(sd_device *device, int *priority); +int device_get_devlink_priority(sd_device *device, int *ret); int device_get_watch_handle(sd_device *device); -int device_get_devnode_mode(sd_device *device, mode_t *mode); -int device_get_devnode_uid(sd_device *device, uid_t *uid); -int device_get_devnode_gid(sd_device *device, gid_t *gid); +int device_get_devnode_mode(sd_device *device, mode_t *ret); +int device_get_devnode_uid(sd_device *device, uid_t *ret); +int device_get_devnode_gid(sd_device *device, gid_t *ret); int device_cache_sysattr_value(sd_device *device, const char *key, char *value); int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value); From 17761fb3bfa494c565683222a707cfa28d14b560 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 14:59:06 +0900 Subject: [PATCH 08/10] sd-device: use ERRNO_IS_DEVICE_ABSENT() at one more place --- src/libsystemd/sd-device/device-private.c | 7 +++---- src/libsystemd/sd-device/sd-device.c | 11 +++++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 6ad49c05a07..47b62e7421a 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -835,10 +835,9 @@ int device_shallow_clone(sd_device *old_device, sd_device **new_device) { return r; } - /* And then read uevent file, but ignore errors, as some devices seem to return a spurious - * error on read, e.g. -ENODEV, and even if ifindex or devnum is set in the above, - * sd_device_get_ifindex() or sd_device_get_devnum() fails. See. #19788. */ - (void) device_read_uevent_file(ret); + r = device_read_uevent_file(ret); + if (r < 0) + return r; *new_device = TAKE_PTR(ret); return 0; diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 6c2d1243ac3..0c5b2a67e33 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -724,11 +724,14 @@ int device_read_uevent_file(sd_device *device) { path = strjoina(syspath, "/uevent"); r = read_full_virtual_file(path, &uevent, &uevent_len); - if (IN_SET(r, -EACCES, -ENOENT)) - /* The uevent files may be write-only, or the device may not have uevent file. */ - return 0; - if (r < 0) + if (r < 0) { + /* The uevent files may be write-only, the device may be already removed, or the device + * may not have the uevent file. */ + if (r == -EACCES || ERRNO_IS_DEVICE_ABSENT(r)) + return 0; + return log_device_debug_errno(device, r, "sd-device: Failed to read uevent file '%s': %m", path); + } for (size_t i = 0; i < uevent_len; i++) switch (state) { From 9c5d7151c14134fc45bf8dc2795fadccb5c3399c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 16:09:11 +0900 Subject: [PATCH 09/10] sd-device: fix possible use-of-uninitialized-value --- src/libsystemd/sd-device/device-private.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 47b62e7421a..42a5467b81e 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -793,7 +793,7 @@ int device_rename(sd_device *device, const char *name) { int device_shallow_clone(sd_device *old_device, sd_device **new_device) { _cleanup_(sd_device_unrefp) sd_device *ret = NULL; - const char *val; + const char *val = NULL; int r; assert(old_device); From 23d20adc05eda10ad0c89203cab36059dfc9da7c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 17 Apr 2022 16:09:57 +0900 Subject: [PATCH 10/10] sd-device: rename arguments and variables --- src/libsystemd/sd-device/device-private.c | 49 +++++++++++------------ src/libsystemd/sd-device/device-private.h | 4 +- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 42a5467b81e..4a5a110364c 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -791,28 +791,28 @@ int device_rename(sd_device *device, const char *name) { return device_add_property_internal(device, "INTERFACE", name); } -int device_shallow_clone(sd_device *old_device, sd_device **new_device) { - _cleanup_(sd_device_unrefp) sd_device *ret = NULL; +int device_shallow_clone(sd_device *device, sd_device **ret) { + _cleanup_(sd_device_unrefp) sd_device *dest = NULL; const char *val = NULL; int r; - assert(old_device); - assert(new_device); + assert(device); + assert(ret); - r = device_new_aux(&ret); + r = device_new_aux(&dest); if (r < 0) return r; - r = device_set_syspath(ret, old_device->syspath, false); + r = device_set_syspath(dest, device->syspath, false); if (r < 0) return r; - (void) sd_device_get_subsystem(old_device, &val); - r = device_set_subsystem(ret, val); + (void) sd_device_get_subsystem(device, &val); + r = device_set_subsystem(dest, val); if (r < 0) return r; if (streq_ptr(val, "drivers")) { - r = free_and_strdup(&ret->driver_subsystem, old_device->driver_subsystem); + r = free_and_strdup(&dest->driver_subsystem, device->driver_subsystem); if (r < 0) return r; } @@ -820,48 +820,47 @@ int device_shallow_clone(sd_device *old_device, sd_device **new_device) { /* The device may be already removed. Let's copy minimal set of information to make * device_get_device_id() work without uevent file. */ - if (sd_device_get_property_value(old_device, "IFINDEX", &val) >= 0) { - r = device_set_ifindex(ret, val); + if (sd_device_get_property_value(device, "IFINDEX", &val) >= 0) { + r = device_set_ifindex(dest, val); if (r < 0) return r; } - if (sd_device_get_property_value(old_device, "MAJOR", &val) >= 0) { + if (sd_device_get_property_value(device, "MAJOR", &val) >= 0) { const char *minor = NULL; - (void) sd_device_get_property_value(old_device, "MINOR", &minor); - r = device_set_devnum(ret, val, minor); + (void) sd_device_get_property_value(device, "MINOR", &minor); + r = device_set_devnum(dest, val, minor); if (r < 0) return r; } - r = device_read_uevent_file(ret); + r = device_read_uevent_file(dest); if (r < 0) return r; - *new_device = TAKE_PTR(ret); + *ret = TAKE_PTR(dest); return 0; } -int device_clone_with_db(sd_device *old_device, sd_device **new_device) { - _cleanup_(sd_device_unrefp) sd_device *ret = NULL; +int device_clone_with_db(sd_device *device, sd_device **ret) { + _cleanup_(sd_device_unrefp) sd_device *dest = NULL; int r; - assert(old_device); - assert(new_device); + assert(device); + assert(ret); - r = device_shallow_clone(old_device, &ret); + r = device_shallow_clone(device, &dest); if (r < 0) return r; - r = device_read_db(ret); + r = device_read_db(dest); if (r < 0) return r; - ret->sealed = true; - - *new_device = TAKE_PTR(ret); + dest->sealed = true; + *ret = TAKE_PTR(dest); return 0; } diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 9b2557a39c7..b6b36b67270 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -51,8 +51,8 @@ int device_get_properties_nulstr(sd_device *device, const uint8_t **nulstr, size int device_get_properties_strv(sd_device *device, char ***strv); int device_rename(sd_device *device, const char *name); -int device_shallow_clone(sd_device *old_device, sd_device **new_device); -int device_clone_with_db(sd_device *old_device, sd_device **new_device); +int device_shallow_clone(sd_device *device, sd_device **ret); +int device_clone_with_db(sd_device *device, sd_device **ret); int device_copy_properties(sd_device *device_dst, sd_device *device_src); int device_tag_index(sd_device *dev, sd_device *dev_old, bool add);