From 47031c8c880a43bff494fcdd1094499f538a8839 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Mon, 7 Dec 2020 13:33:46 -0800 Subject: [PATCH] ueventd: add no_fnm_pathname option If a `*` appears within (but not at the end) of a /dev or /sys path in a ueventd.rc file, then that path is matched with fnmatch() using the FNM_PATHNAME, which means `*` will not match `/`. That is not always the intended behavior and this change creates the no_fnm_pathname option, which will not use the FNM_PATHNAME flag and will have `*` match `/`. Bug: 172880724 Test: these unit tests Change-Id: I85b813d89237dbf3af47564e5cbf6806df5d412f --- init/README.ueventd.md | 14 +++++++++++--- init/devices.cpp | 15 +++++++++++---- init/devices.h | 7 ++++--- init/devices_test.cpp | 33 ++++++++++++++++++++++++++------- init/ueventd_parser.cpp | 21 +++++++++++++++------ init/ueventd_parser_test.cpp | 33 +++++++++++++++++---------------- 6 files changed, 84 insertions(+), 39 deletions(-) diff --git a/init/README.ueventd.md b/init/README.ueventd.md index 4363f3c89..97fb515f9 100644 --- a/init/README.ueventd.md +++ b/init/README.ueventd.md @@ -32,7 +32,7 @@ for the node path: The permissions can be modified using a ueventd.rc script and a line that beings with `/dev`. These lines take the format of - devname mode uid gid + devname mode uid gid [options] For example /dev/null 0666 root root @@ -70,7 +70,7 @@ Ueventd by default takes no action for `/sys`, however it can be instructed to s certain files in `/sys` when matching uevents are generated. This is done using a ueventd.rc script and a line that begins with `/sys`. These lines take the format of - nodename attr mode uid gid + nodename attr mode uid gid [options] For example /sys/devices/system/cpu/cpu* cpufreq/scaling_max_freq 0664 system system @@ -78,7 +78,15 @@ When a uevent that matches the pattern `/sys/devices/system/cpu/cpu*` is sent, t attribute, `cpufreq/scaling_max_freq`, will have its mode set to `0664`, its user to to `system` and its group set to `system`. -Note that `*` matches as a wildcard and can be used anywhere in a path. +## Path matching +---------------- +The path for a `/dev` or `/sys` entry can contain a `*` anywhere in the path. +1. If the only `*` appears at the end of the string or if the _options_ parameter is set to +`no_fnm_pathname`, ueventd matches the entry by `fnmatch(entry_path, incoming_path, 0)` +2. Otherwise, ueventd matches the entry by `fnmatch(entry_path, incoming_path, FNM_PATHNAME)` + +See the [man page for fnmatch](https://www.man7.org/linux/man-pages/man3/fnmatch.3.html) for more +details. ## Firmware loading ---------------- diff --git a/init/devices.cpp b/init/devices.cpp index 5888c063f..a7c80c1ff 100644 --- a/init/devices.cpp +++ b/init/devices.cpp @@ -124,8 +124,15 @@ static bool FindDmDevice(const std::string& path, std::string* name, std::string return true; } -Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid) - : name_(name), perm_(perm), uid_(uid), gid_(gid), prefix_(false), wildcard_(false) { +Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid, + bool no_fnm_pathname) + : name_(name), + perm_(perm), + uid_(uid), + gid_(gid), + prefix_(false), + wildcard_(false), + no_fnm_pathname_(no_fnm_pathname) { // Set 'prefix_' or 'wildcard_' based on the below cases: // // 1) No '*' in 'name' -> Neither are set and Match() checks a given path for strict @@ -136,7 +143,6 @@ Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t // // 3) '*' appears elsewhere -> 'wildcard_' is set to true and Match() uses fnmatch() // with FNM_PATHNAME to compare 'name' to a given path. - auto wildcard_position = name_.find('*'); if (wildcard_position != std::string::npos) { if (wildcard_position == name_.length() - 1) { @@ -150,7 +156,8 @@ Permissions::Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t bool Permissions::Match(const std::string& path) const { if (prefix_) return StartsWith(path, name_); - if (wildcard_) return fnmatch(name_.c_str(), path.c_str(), FNM_PATHNAME) == 0; + if (wildcard_) + return fnmatch(name_.c_str(), path.c_str(), no_fnm_pathname_ ? 0 : FNM_PATHNAME) == 0; return path == name_; } diff --git a/init/devices.h b/init/devices.h index 05d64da6e..d70d746e8 100644 --- a/init/devices.h +++ b/init/devices.h @@ -38,7 +38,7 @@ class Permissions { public: friend void TestPermissions(const Permissions& expected, const Permissions& test); - Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid); + Permissions(const std::string& name, mode_t perm, uid_t uid, gid_t gid, bool no_fnm_pathname); bool Match(const std::string& path) const; @@ -56,6 +56,7 @@ class Permissions { gid_t gid_; bool prefix_; bool wildcard_; + bool no_fnm_pathname_; }; class SysfsPermissions : public Permissions { @@ -63,8 +64,8 @@ class SysfsPermissions : public Permissions { friend void TestSysfsPermissions(const SysfsPermissions& expected, const SysfsPermissions& test); SysfsPermissions(const std::string& name, const std::string& attribute, mode_t perm, uid_t uid, - gid_t gid) - : Permissions(name, perm, uid, gid), attribute_(attribute) {} + gid_t gid, bool no_fnm_pathname) + : Permissions(name, perm, uid, gid, no_fnm_pathname), attribute_(attribute) {} bool MatchWithSubsystem(const std::string& path, const std::string& subsystem) const; void SetPermissions(const std::string& path) const; diff --git a/init/devices_test.cpp b/init/devices_test.cpp index c408bc130..e7bac68bc 100644 --- a/init/devices_test.cpp +++ b/init/devices_test.cpp @@ -221,7 +221,7 @@ TEST(device_handler, sanitize_onebad) { TEST(device_handler, DevPermissionsMatchNormal) { // Basic from ueventd.rc // /dev/null 0666 root root - Permissions permissions("/dev/null", 0666, 0, 0); + Permissions permissions("/dev/null", 0666, 0, 0, false); EXPECT_TRUE(permissions.Match("/dev/null")); EXPECT_FALSE(permissions.Match("/dev/nullsuffix")); EXPECT_FALSE(permissions.Match("/dev/nul")); @@ -233,7 +233,7 @@ TEST(device_handler, DevPermissionsMatchNormal) { TEST(device_handler, DevPermissionsMatchPrefix) { // Prefix from ueventd.rc // /dev/dri/* 0666 root graphics - Permissions permissions("/dev/dri/*", 0666, 0, 1000); + Permissions permissions("/dev/dri/*", 0666, 0, 1000, false); EXPECT_TRUE(permissions.Match("/dev/dri/some_dri_device")); EXPECT_TRUE(permissions.Match("/dev/dri/some_other_dri_device")); EXPECT_TRUE(permissions.Match("/dev/dri/")); @@ -246,7 +246,7 @@ TEST(device_handler, DevPermissionsMatchPrefix) { TEST(device_handler, DevPermissionsMatchWildcard) { // Wildcard example // /dev/device*name 0666 root graphics - Permissions permissions("/dev/device*name", 0666, 0, 1000); + Permissions permissions("/dev/device*name", 0666, 0, 1000, false); EXPECT_TRUE(permissions.Match("/dev/devicename")); EXPECT_TRUE(permissions.Match("/dev/device123name")); EXPECT_TRUE(permissions.Match("/dev/deviceabcname")); @@ -260,13 +260,31 @@ TEST(device_handler, DevPermissionsMatchWildcard) { TEST(device_handler, DevPermissionsMatchWildcardPrefix) { // Wildcard+Prefix example // /dev/device*name* 0666 root graphics - Permissions permissions("/dev/device*name*", 0666, 0, 1000); + Permissions permissions("/dev/device*name*", 0666, 0, 1000, false); EXPECT_TRUE(permissions.Match("/dev/devicename")); EXPECT_TRUE(permissions.Match("/dev/device123name")); EXPECT_TRUE(permissions.Match("/dev/deviceabcname")); EXPECT_TRUE(permissions.Match("/dev/device123namesomething")); // FNM_PATHNAME doesn't match '/' with * EXPECT_FALSE(permissions.Match("/dev/device123name/something")); + EXPECT_FALSE(permissions.Match("/dev/device/1/2/3name/something")); + EXPECT_FALSE(permissions.Match("/dev/deviceame")); + EXPECT_EQ(0666U, permissions.perm()); + EXPECT_EQ(0U, permissions.uid()); + EXPECT_EQ(1000U, permissions.gid()); +} + +TEST(device_handler, DevPermissionsMatchWildcardPrefix_NoFnmPathName) { + // Wildcard+Prefix example with no_fnm_pathname + // /dev/device*name* 0666 root graphics + Permissions permissions("/dev/device*name*", 0666, 0, 1000, true); + EXPECT_TRUE(permissions.Match("/dev/devicename")); + EXPECT_TRUE(permissions.Match("/dev/device123name")); + EXPECT_TRUE(permissions.Match("/dev/deviceabcname")); + EXPECT_TRUE(permissions.Match("/dev/device123namesomething")); + // With NoFnmPathName, the below matches, unlike DevPermissionsMatchWildcardPrefix. + EXPECT_TRUE(permissions.Match("/dev/device123name/something")); + EXPECT_TRUE(permissions.Match("/dev/device/1/2/3name/something")); EXPECT_FALSE(permissions.Match("/dev/deviceame")); EXPECT_EQ(0666U, permissions.perm()); EXPECT_EQ(0U, permissions.uid()); @@ -275,7 +293,8 @@ TEST(device_handler, DevPermissionsMatchWildcardPrefix) { TEST(device_handler, SysfsPermissionsMatchWithSubsystemNormal) { // /sys/devices/virtual/input/input* enable 0660 root input - SysfsPermissions permissions("/sys/devices/virtual/input/input*", "enable", 0660, 0, 1001); + SysfsPermissions permissions("/sys/devices/virtual/input/input*", "enable", 0660, 0, 1001, + false); EXPECT_TRUE(permissions.MatchWithSubsystem("/sys/devices/virtual/input/input0", "input")); EXPECT_FALSE(permissions.MatchWithSubsystem("/sys/devices/virtual/input/not_input0", "input")); EXPECT_EQ(0660U, permissions.perm()); @@ -285,7 +304,7 @@ TEST(device_handler, SysfsPermissionsMatchWithSubsystemNormal) { TEST(device_handler, SysfsPermissionsMatchWithSubsystemClass) { // /sys/class/input/event* enable 0660 root input - SysfsPermissions permissions("/sys/class/input/event*", "enable", 0660, 0, 1001); + SysfsPermissions permissions("/sys/class/input/event*", "enable", 0660, 0, 1001, false); EXPECT_TRUE(permissions.MatchWithSubsystem( "/sys/devices/soc.0/f9924000.i2c/i2c-2/2-0020/input/input0/event0", "input")); EXPECT_FALSE(permissions.MatchWithSubsystem( @@ -299,7 +318,7 @@ TEST(device_handler, SysfsPermissionsMatchWithSubsystemClass) { TEST(device_handler, SysfsPermissionsMatchWithSubsystemBus) { // /sys/bus/i2c/devices/i2c-* enable 0660 root input - SysfsPermissions permissions("/sys/bus/i2c/devices/i2c-*", "enable", 0660, 0, 1001); + SysfsPermissions permissions("/sys/bus/i2c/devices/i2c-*", "enable", 0660, 0, 1001, false); EXPECT_TRUE(permissions.MatchWithSubsystem("/sys/devices/soc.0/f9967000.i2c/i2c-5", "i2c")); EXPECT_FALSE(permissions.MatchWithSubsystem("/sys/devices/soc.0/f9967000.i2c/not-i2c", "i2c")); EXPECT_FALSE( diff --git a/init/ueventd_parser.cpp b/init/ueventd_parser.cpp index 09dce4481..357895793 100644 --- a/init/ueventd_parser.cpp +++ b/init/ueventd_parser.cpp @@ -33,12 +33,12 @@ Result ParsePermissionsLine(std::vector&& args, std::vector* out_sysfs_permissions, std::vector* out_dev_permissions) { bool is_sysfs = out_sysfs_permissions != nullptr; - if (is_sysfs && args.size() != 5) { - return Error() << "/sys/ lines must have 5 entries"; + if (is_sysfs && !(args.size() == 5 || args.size() == 6)) { + return Error() << "/sys/ lines must have 5 or 6 entries"; } - if (!is_sysfs && args.size() != 4) { - return Error() << "/dev/ lines must have 4 entries"; + if (!is_sysfs && !(args.size() == 4 || args.size() == 5)) { + return Error() << "/dev/ lines must have 4 or 5 entries"; } auto it = args.begin(); @@ -69,10 +69,19 @@ Result ParsePermissionsLine(std::vector&& args, } gid_t gid = grp->gr_gid; + bool no_fnm_pathname = false; + if (it != args.end()) { + std::string& flags = *it++; + if (flags != "no_fnm_pathname") { + return Error() << "invalid option '" << flags << "', only no_fnm_pathname is supported"; + } + no_fnm_pathname = true; + } + if (is_sysfs) { - out_sysfs_permissions->emplace_back(name, sysfs_attribute, perm, uid, gid); + out_sysfs_permissions->emplace_back(name, sysfs_attribute, perm, uid, gid, no_fnm_pathname); } else { - out_dev_permissions->emplace_back(name, perm, uid, gid); + out_dev_permissions->emplace_back(name, perm, uid, gid, no_fnm_pathname); } return {}; } diff --git a/init/ueventd_parser_test.cpp b/init/ueventd_parser_test.cpp index 172ba0b38..b604c53fc 100644 --- a/init/ueventd_parser_test.cpp +++ b/init/ueventd_parser_test.cpp @@ -104,21 +104,21 @@ TEST(ueventd_parser, Permissions) { /dev/graphics/* 0660 root graphics /dev/*/test 0660 root system -/sys/devices/platform/trusty.* trusty_version 0440 root log -/sys/devices/virtual/input/input enable 0660 root input -/sys/devices/virtual/*/input poll_delay 0660 root input +/sys/devices/platform/trusty.* trusty_version 0440 root log +/sys/devices/virtual/input/input enable 0660 root input +/sys/devices/virtual/*/input poll_delay 0660 root input no_fnm_pathname )"; auto permissions = std::vector{ - {"/dev/rtc0", 0640, AID_SYSTEM, AID_SYSTEM}, - {"/dev/graphics/*", 0660, AID_ROOT, AID_GRAPHICS}, - {"/dev/*/test", 0660, AID_ROOT, AID_SYSTEM}, + {"/dev/rtc0", 0640, AID_SYSTEM, AID_SYSTEM, false}, + {"/dev/graphics/*", 0660, AID_ROOT, AID_GRAPHICS, false}, + {"/dev/*/test", 0660, AID_ROOT, AID_SYSTEM, false}, }; auto sysfs_permissions = std::vector{ - {"/sys/devices/platform/trusty.*", "trusty_version", 0440, AID_ROOT, AID_LOG}, - {"/sys/devices/virtual/input/input", "enable", 0660, AID_ROOT, AID_INPUT}, - {"/sys/devices/virtual/*/input", "poll_delay", 0660, AID_ROOT, AID_INPUT}, + {"/sys/devices/platform/trusty.*", "trusty_version", 0440, AID_ROOT, AID_LOG, false}, + {"/sys/devices/virtual/input/input", "enable", 0660, AID_ROOT, AID_INPUT, false}, + {"/sys/devices/virtual/*/input", "poll_delay", 0660, AID_ROOT, AID_INPUT, true}, }; TestUeventdFile(ueventd_file, {{}, sysfs_permissions, permissions, {}, {}}); @@ -240,7 +240,7 @@ subsystem test_devpath_dirname dirname /dev/graphics /dev/*/test 0660 root system -/sys/devices/virtual/*/input poll_delay 0660 root input +/sys/devices/virtual/*/input poll_delay 0660 root input no_fnm_pathname firmware_directories /more external_firmware_handler /devices/path/firmware/firmware001.bin root /vendor/bin/touch.sh @@ -259,15 +259,15 @@ parallel_restorecon enabled {"test_devpath_dirname", Subsystem::DEVNAME_UEVENT_DEVPATH, "/dev/graphics"}}; auto permissions = std::vector{ - {"/dev/rtc0", 0640, AID_SYSTEM, AID_SYSTEM}, - {"/dev/graphics/*", 0660, AID_ROOT, AID_GRAPHICS}, - {"/dev/*/test", 0660, AID_ROOT, AID_SYSTEM}, + {"/dev/rtc0", 0640, AID_SYSTEM, AID_SYSTEM, false}, + {"/dev/graphics/*", 0660, AID_ROOT, AID_GRAPHICS, false}, + {"/dev/*/test", 0660, AID_ROOT, AID_SYSTEM, false}, }; auto sysfs_permissions = std::vector{ - {"/sys/devices/platform/trusty.*", "trusty_version", 0440, AID_ROOT, AID_LOG}, - {"/sys/devices/virtual/input/input", "enable", 0660, AID_ROOT, AID_INPUT}, - {"/sys/devices/virtual/*/input", "poll_delay", 0660, AID_ROOT, AID_INPUT}, + {"/sys/devices/platform/trusty.*", "trusty_version", 0440, AID_ROOT, AID_LOG, false}, + {"/sys/devices/virtual/input/input", "enable", 0660, AID_ROOT, AID_INPUT, false}, + {"/sys/devices/virtual/*/input", "poll_delay", 0660, AID_ROOT, AID_INPUT, true}, }; auto firmware_directories = std::vector{ @@ -299,6 +299,7 @@ firmware_directories #no directory listed /sys/devices/platform/trusty.* trusty_version badmode root log /sys/devices/platform/trusty.* trusty_version 0440 baduidbad log /sys/devices/platform/trusty.* trusty_version 0440 root baduidbad +/sys/devices/platform/trusty.* trusty_version 0440 root root bad_option uevent_socket_rcvbuf_size blah