Merge pull request #27169 from yuwata/udev-rule-refuse-unsafe-path

sd-device,udev: refuse unsafe path in SYMLINK= and TAG=
This commit is contained in:
Zbigniew Jędrzejewski-Szmek
2023-04-11 14:43:50 +02:00
committed by GitHub
9 changed files with 206 additions and 58 deletions

View File

@@ -38,6 +38,10 @@ 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", ATTRS{nsid}=="?*", ENV{ID_NSID}="$attr{nsid}"
# obsolete symlink with non-escaped characters, kept for backward compatiblity
KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \
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}"
# obsolete symlink that might get overridden on adding a new nvme controller, kept for backward compatibility
KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \
OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}"
@@ -48,10 +52,14 @@ KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ATTRS{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", ATTRS{nsid}=="?*", ENV{ID_NSID}="$attr{nsid}"
# obsolete symlink with non-escaped characters, kept for backward compatiblity
KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \
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"
# obsolete symlink that might get overridden on adding a new nvme controller, kept for backward compatibility
KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \
OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n"
KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_NSID}=="?*", \
KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_NSID}=="?*", \
OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}_$env{ID_NSID}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n"
# virtio-blk

View File

@@ -351,6 +351,8 @@ static int device_amend(sd_device *device, const char *key, const char *value) {
return r;
if (r == 0)
break;
if (isempty(word))
continue;
r = device_add_tag(device, word, streq(key, "CURRENT_TAGS"));
if (r < 0)

View File

@@ -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);

View File

@@ -1439,7 +1439,7 @@ _public_ int sd_device_get_diskseq(sd_device *device, uint64_t *ret) {
static bool is_valid_tag(const char *tag) {
assert(tag);
return !strchr(tag, ':') && !strchr(tag, ' ');
return in_charset(tag, ALPHANUMERICAL "-_") && filename_is_valid(tag);
}
int device_add_tag(sd_device *device, const char *tag, bool both) {
@@ -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) {
@@ -2270,7 +2294,7 @@ int device_cache_sysattr_value(sd_device *device, const char *key, char *value)
return -ENOMEM;
}
r = hashmap_ensure_put(&device->sysattr_values, &string_hash_ops_free_free, new_key, value);
r = hashmap_ensure_put(&device->sysattr_values, &path_hash_ops_free_free, new_key, value);
if (r < 0)
return r;

View File

@@ -35,7 +35,7 @@
#include "user-util.h"
#include "virt.h"
#define RULES_DIRS (const char* const*) CONF_PATHS_STRV("udev/rules.d")
#define RULES_DIRS ((const char* const*) CONF_PATHS_STRV("udev/rules.d"))
typedef enum {
OP_MATCH, /* == */
@@ -2487,16 +2487,14 @@ static int udev_rule_apply_token_to_event(
if (token->op == OP_ASSIGN)
device_cleanup_tags(dev);
if (buf[strspn(buf, ALPHANUMERICAL "-_")] != '\0') {
log_event_error(dev, token, "Invalid tag name '%s', ignoring", buf);
break;
}
if (token->op == OP_REMOVE)
device_remove_tag(dev, buf);
else {
r = device_add_tag(dev, buf, true);
if (r == -ENOMEM)
return log_oom();
if (r < 0)
return log_event_error_errno(dev, token, r, "Failed to add tag '%s': %m", buf);
log_event_warning_errno(dev, token, r, "Failed to add tag '%s', ignoring: %m", buf);
}
break;
}
@@ -2541,7 +2539,7 @@ static int udev_rule_apply_token_to_event(
break;
}
case TK_A_DEVLINK: {
char buf[UDEV_PATH_SIZE], *p;
char buf[UDEV_PATH_SIZE];
bool truncated;
size_t count;
@@ -2554,17 +2552,18 @@ static int udev_rule_apply_token_to_event(
if (IN_SET(token->op, OP_ASSIGN, OP_ASSIGN_FINAL))
device_cleanup_devlinks(dev);
/* allow multiple symlinks separated by spaces */
(void) udev_event_apply_format(event, token->value, buf, sizeof(buf), event->esc != ESCAPE_NONE, &truncated);
(void) udev_event_apply_format(event, token->value, buf, sizeof(buf),
/* replace_whitespace = */ event->esc != ESCAPE_NONE, &truncated);
if (truncated) {
log_event_truncated(dev, token, "symbolic link path", token->value, "SYMLINK", /* is_match = */ false);
break;
}
/* By default or string_escape=none, allow multiple symlinks separated by spaces. */
if (event->esc == ESCAPE_UNSET)
count = udev_replace_chars(buf, "/ ");
count = udev_replace_chars(buf, /* allow = */ "/ ");
else if (event->esc == ESCAPE_REPLACE)
count = udev_replace_chars(buf, "/");
count = udev_replace_chars(buf, /* allow = */ "/");
else
count = 0;
if (count > 0)
@@ -2572,32 +2571,36 @@ static int udev_rule_apply_token_to_event(
"Replaced %zu character(s) from result of SYMLINK=\"%s\"",
count, token->value);
p = skip_leading_chars(buf, NULL);
while (!isempty(p)) {
char filename[UDEV_PATH_SIZE], *next;
for (const char *p = buf;;) {
_cleanup_free_ char *path = NULL;
next = strchr(p, ' ');
if (next) {
*next++ = '\0';
next = skip_leading_chars(next, NULL);
r = extract_first_word(&p, &path, NULL, EXTRACT_RETAIN_ESCAPE);
if (r == -ENOMEM)
return log_oom();
if (r < 0) {
log_warning_errno(r, "Failed to extract first path in SYMLINK=, ignoring: %m");
break;
}
strscpyl_full(filename, sizeof(filename), &truncated, "/dev/", p, NULL);
if (truncated)
continue;
if (r == 0)
break;
if (token->op == OP_REMOVE) {
device_remove_devlink(dev, filename);
log_event_debug(dev, token, "Dropped SYMLINK '%s'", p);
} else {
r = device_add_devlink(dev, filename);
r = device_remove_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", filename);
log_event_debug(dev, token, "Added SYMLINK '%s'", p);
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)
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);
}
p = next;
}
break;
}

View File

@@ -88,6 +88,8 @@ _image_cleanup() {
# Clean up certain "problematic" files which may be left over by failing tests
: >"${initdir:?}/etc/fstab"
: >"${initdir:?}/etc/crypttab"
# Clear previous assignment
QEMU_OPTIONS_ARRAY=()
}
test_run_one() {
@@ -193,15 +195,34 @@ testcase_nvme_basic() {
local i
local qemu_opts=()
for i in {0..27}; do
for (( i = 0; i < 5; i++ )); do
qemu_opts+=(
"-device nvme,drive=nvme$i,serial=deadbeef$i,num_queues=8"
"-drive format=raw,cache=unsafe,file=${TESTDIR:?}/disk$i.img,if=none,id=nvme$i"
"-device" "nvme,drive=nvme$i,serial=deadbeef$i,num_queues=8"
"-drive" "format=raw,cache=unsafe,file=${TESTDIR:?}/disk$i.img,if=none,id=nvme$i"
)
done
for (( i = 5; i < 10; i++ )); do
qemu_opts+=(
"-device" "nvme,drive=nvme$i,serial= deadbeef $i ,num_queues=8"
"-drive" "format=raw,cache=unsafe,file=${TESTDIR:?}/disk$i.img,if=none,id=nvme$i"
)
done
for (( i = 10; i < 15; i++ )); do
qemu_opts+=(
"-device" "nvme,drive=nvme$i,serial= dead/beef/$i ,num_queues=8"
"-drive" "format=raw,cache=unsafe,file=${TESTDIR:?}/disk$i.img,if=none,id=nvme$i"
)
done
for (( i = 15; i < 20; i++ )); do
qemu_opts+=(
"-device" "nvme,drive=nvme$i,serial=dead/../../beef/$i,num_queues=8"
"-drive" "format=raw,cache=unsafe,file=${TESTDIR:?}/disk$i.img,if=none,id=nvme$i"
)
done
KERNEL_APPEND="systemd.setenv=TEST_FUNCTION_NAME=${FUNCNAME[0]} ${USER_KERNEL_APPEND:-}"
QEMU_OPTIONS="${qemu_opts[*]} ${USER_QEMU_OPTIONS:-}"
QEMU_OPTIONS="${USER_QEMU_OPTIONS}"
QEMU_OPTIONS_ARRAY=("${qemu_opts[@]}")
test_run_one "${1:?}"
}

View File

@@ -512,6 +512,7 @@ run_qemu() {
read -ra user_qemu_options <<< "$QEMU_OPTIONS"
qemu_options+=("${user_qemu_options[@]}")
fi
qemu_options+=(${QEMU_OPTIONS_ARRAY:+"${QEMU_OPTIONS_ARRAY[@]}"})
if [[ -n "${KERNEL_APPEND:=}" ]]; then
local user_kernel_append

View File

@@ -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
},
{
@@ -1784,9 +1808,9 @@ EOF
not_exp_name => "bad",
}],
rules => <<EOF
KERNEL=="sda", TAG=""
TAGS=="|foo", SYMLINK+="found"
TAGS=="aaa|bbb", SYMLINK+="bad"
KERNEL=="sda", ENV{HOGE}=""
ENV{HOGE}=="|foo", SYMLINK+="found"
ENV{HOGE}=="aaa|bbb", SYMLINK+="bad"
EOF
},
{
@@ -1812,9 +1836,9 @@ EOF
not_exp_name => "bad",
}],
rules => <<EOF
KERNEL=="sda", TAG=""
TAGS=="foo||bar", SYMLINK+="found"
TAGS=="aaa|bbb", SYMLINK+="bad"
KERNEL=="sda", ENV{HOGE}=""
ENV{HOGE}=="foo||bar", SYMLINK+="found"
ENV{HOGE}=="aaa|bbb", SYMLINK+="bad"
EOF
},
{
@@ -1840,9 +1864,9 @@ EOF
not_exp_name => "bad",
}],
rules => <<EOF
KERNEL=="sda", TAG=""
TAGS=="foo|", SYMLINK+="found"
TAGS=="aaa|bbb", SYMLINK+="bad"
KERNEL=="sda", ENV{HOGE}=""
ENV{HOGE}=="foo|", SYMLINK+="found"
ENV{HOGE}=="aaa|bbb", SYMLINK+="bad"
EOF
},
{
@@ -1857,6 +1881,25 @@ EOF
KERNEL=="sda", TAG="c"
TAGS=="foo||bar||c", SYMLINK+="found"
TAGS=="aaa||bbb||ccc", SYMLINK+="bad"
EOF
},
{
desc => "TAG refuses invalid string",
devices => [
{
devpath => "/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda",
exp_links => ["valid", "found"],
not_exp_links => ["empty", "invalid_char", "path", "bad", "bad2"],
}],
rules => <<EOF
KERNEL=="sda", TAG+="", TAG+="invalid.char", TAG+="path/is/also/invalid", TAG+="valid"
TAGS=="", SYMLINK+="empty"
TAGS=="invalid.char", SYMLINK+="invalid_char"
TAGS=="path/is/also/invalid", SYMLINK+="path"
TAGS=="valid", SYMLINK+="valid"
TAGS=="valid|", SYMLINK+="found"
TAGS=="aaa|", SYMLINK+="bad"
TAGS=="aaa|bbb", SYMLINK+="bad2"
EOF
},
{

View File

@@ -174,8 +174,54 @@ testcase_megasas2_basic() {
}
testcase_nvme_basic() {
local expected_symlinks=()
local i
for (( i = 0; i < 5; i++ )); do
expected_symlinks+=(
# both replace mode provides the same devlink
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_deadbeef"$i"
# with nsid
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_deadbeef"$i"_1
)
done
for (( i = 5; i < 10; i++ )); do
expected_symlinks+=(
# old replace mode
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl__deadbeef_"$i"
# newer replace mode
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_____deadbeef__"$i"
# with nsid
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_____deadbeef__"$i"_1
)
done
for (( i = 10; i < 15; i++ )); do
expected_symlinks+=(
# old replace mode does not provide devlink, as serial contains "/"
# newer replace mode
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_____dead_beef_"$i"
# with nsid
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_____dead_beef_"$i"_1
)
done
for (( i = 15; i < 20; i++ )); do
expected_symlinks+=(
# old replace mode does not provide devlink, as serial contains "/"
# newer replace mode
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_dead_.._.._beef_"$i"
# with nsid
/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_dead_.._.._beef_"$i"_1
)
done
udevadm settle
ls /dev/disk/by-id
for i in "${expected_symlinks[@]}"; do
udevadm wait --settle --timeout=30 "$i"
done
lsblk --noheadings | grep "^nvme"
[[ "$(lsblk --noheadings | grep -c "^nvme")" -ge 28 ]]
[[ "$(lsblk --noheadings | grep -c "^nvme")" -ge 20 ]]
}
testcase_nvme_subsystem() {