This is an automated email from the ASF dual-hosted git repository. bmahler pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 45d290aeff6912c8e6a4b1a7358c4e9772c447b4 Author: Jason Zhou <[email protected]> AuthorDate: Fri Jul 26 01:45:30 2024 -0400 [cgroups2] Add allow / deny list normalization validation. Currently we assume that a device state is normalized before using it for generating ebpf files. However, we have not been enforcing these constraints on the device access state. We enforce some basic validation on cgroups2::configure on the state to ensure that we are able to generate a correct ebpf program. If the lists are not normalized, we generate incorrect programs! An allow or deny list is 'normalized' iff everything below are true: 1. No entries have empty accesses specified. 2. No two entries on the same list can have the same selector (type, major & minor numbers). 3. No two entries on the same list can be encompassed by the other entry. See Entry::encompassed. This patch adds helpers to check if a device state is normalized, and will only allow users to create new CgroupDeviceAccess instances using a helper that checks that the allow and deny lists are normalized. A new helper function is added to check if an entry would be granted access, and requires the state to be normalized. Review: https://reviews.apache.org/r/75099/ --- src/linux/cgroups2.cpp | 12 ++ src/linux/cgroups2.hpp | 10 +- .../device_manager/device_manager.cpp | 138 +++++++++++++++++++-- .../device_manager/device_manager.hpp | 12 ++ src/tests/containerizer/cgroups2_tests.cpp | 42 +++++++ src/tests/device_manager_tests.cpp | 95 ++++++++++++++ 6 files changed, 301 insertions(+), 8 deletions(-) diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp index ac4678261..2c8290727 100644 --- a/src/linux/cgroups2.cpp +++ b/src/linux/cgroups2.cpp @@ -41,6 +41,8 @@ #include "linux/ebpf.hpp" #include "linux/fs.hpp" +#include "slave/containerizer/device_manager/device_manager.hpp" + using std::ostream; using std::set; using std::string; @@ -1465,6 +1467,16 @@ Try<Nothing> configure( const vector<Entry>& allow, const vector<Entry>& deny) { + using mesos::internal::slave::DeviceManager; + DeviceManager::CgroupDeviceAccess state = + CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})); + state.allow_list = allow; + state.deny_list = deny; + if (!state.normalized()) { + return Error( + "Failed to validate arguments: allow or deny lists are not normalized."); + } + Try<ebpf::Program> program = DeviceProgram::build(allow, deny); if (program.isError()) { diff --git a/src/linux/cgroups2.hpp b/src/linux/cgroups2.hpp index accaebdad..a814bc03c 100644 --- a/src/linux/cgroups2.hpp +++ b/src/linux/cgroups2.hpp @@ -434,7 +434,15 @@ using cgroups::devices::Entry; // are hierarchical. I.e. if a parent cgroup does not allow an access then // 'this' cgroup will be denied access. // For access to be granted, the requested access must match an entry in the -// allow list, and not match with any entry on the deny list. +// allow list, and not match with any entry on the deny list. If the device +// type, major, minor numbers match with an entry on the deny list, there must +// be no overlap between the requested access and the deny entry's accesses +// +// We additionally enforce the following constraints on both allow and deny: +// 1. No Entry can have no accesses specified +// 2. No two entries on the same list can have the same type, major & minor +// numbers. +// 3. No two entries on the same list can be encompassed by the other entry. Try<Nothing> configure( const std::string& cgroup, const std::vector<Entry>& allow, diff --git a/src/slave/containerizer/device_manager/device_manager.cpp b/src/slave/containerizer/device_manager/device_manager.cpp index 4c9b86393..6f013d4bc 100644 --- a/src/slave/containerizer/device_manager/device_manager.cpp +++ b/src/slave/containerizer/device_manager/device_manager.cpp @@ -91,9 +91,21 @@ public: } } } + auto it = device_access_per_cgroup.find(cgroup); + if (it != device_access_per_cgroup.end()) { + return Failure( + "Failed to configure allow and deny devices: cgroup entry already " + "exists."); + } else { + auto result = device_access_per_cgroup.emplace( + cgroup, + CHECK_NOTERROR( + DeviceManager::CgroupDeviceAccess::create(allow_list, deny_list))); + if (!result.second) { + return Failure("Failed to insert new element for key '" + cgroup + "'"); + } + } - device_access_per_cgroup[cgroup].allow_list = allow_list; - device_access_per_cgroup[cgroup].deny_list = deny_list; Try<Nothing> commit = commit_device_access_changes(cgroup); if (commit.isError()) { @@ -124,10 +136,21 @@ public: } } - device_access_per_cgroup[cgroup] = DeviceManager::apply_diff( - device_access_per_cgroup[cgroup], - non_wildcard_additions, - non_wildcard_removals); + auto it = device_access_per_cgroup.find(cgroup); + if (it != device_access_per_cgroup.end()) { + it->second = DeviceManager::apply_diff( + it->second, non_wildcard_additions, non_wildcard_removals); + } else { + auto result = device_access_per_cgroup.emplace( + cgroup, + DeviceManager::apply_diff( + CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})), + non_wildcard_additions, + non_wildcard_removals)); + if (!result.second) { + return Failure("Failed to insert new element for key '" + cgroup + "'"); + } + } Try<Nothing> commit = commit_device_access_changes(cgroup); if (commit.isError()) { @@ -149,7 +172,7 @@ public: { return device_access_per_cgroup.contains(cgroup) ? device_access_per_cgroup.at(cgroup) - : DeviceManager::CgroupDeviceAccess(); + : CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})); } private: @@ -174,6 +197,107 @@ private: }; +DeviceManager::CgroupDeviceAccess::CgroupDeviceAccess( + const std::vector<cgroups::devices::Entry>& _allow_list, + const std::vector<cgroups::devices::Entry>& _deny_list) + : allow_list(_allow_list), deny_list(_deny_list) {} + + +bool DeviceManager::CgroupDeviceAccess::normalized() const +{ + auto has_empties = [](const vector<Entry>& entries) { + foreach (const Entry& entry, entries) { + if (entry.access.none()) { + return true; + } + } + return false; + }; + + if (has_empties(allow_list) || has_empties(deny_list)) { + return false; + } + + auto has_duplicate_selectors = [](const vector<Entry>& entries) { + hashset<string> seen; + foreach (const Entry& entry, entries) { + Entry no_access = entry; + no_access.access.write = false; + no_access.access.read = false; + no_access.access.mknod = false; + string item = stringify(no_access); + if (seen.contains(item)) { + return true; + } + seen.insert(item); + } + return false; + }; + + if ( + has_duplicate_selectors(allow_list) || has_duplicate_selectors(deny_list)) { + return false; + } + + auto has_encompassed_entries = [](const vector<Entry>& entries) { + foreach (const Entry& entry, entries) { + foreach (const Entry& other, entries) { + if ( + (!cgroups::devices::operator==(entry, other)) && + other.encompasses(entry)) { + return true; + } + } + } + return false; + }; + + if ( + has_encompassed_entries(allow_list) || has_encompassed_entries(deny_list)) { + return false; + } + return true; +} + + +Try<DeviceManager::CgroupDeviceAccess> +DeviceManager::CgroupDeviceAccess::create( + const vector<Entry>& allow_list, const vector<Entry>& deny_list) +{ + CgroupDeviceAccess res(allow_list, deny_list); + if (!res.normalized()) { + return Error( + "Failed to create CgroupDeviceAccess: The allow or deny list is not " + "normalized."); + } + return res; +} + + +bool DeviceManager::CgroupDeviceAccess::is_access_granted( + const Entry& query) const +{ + CHECK(normalized()); + bool encompassed_by_allow = false; + foreach (const Entry& allow, allow_list) { + if (allow.encompasses(query)) { + encompassed_by_allow = true; + } + } + if (!encompassed_by_allow) { + return false; + } + foreach (const Entry& deny, deny_list) { + if ( + deny.selector.encompasses(query.selector) && + deny.access.overlaps(query.access)) { + return false; + } + } + return true; +} + + Try<DeviceManager*> DeviceManager::create(const Flags& flags) { return new DeviceManager( diff --git a/src/slave/containerizer/device_manager/device_manager.hpp b/src/slave/containerizer/device_manager/device_manager.hpp index 7c8523d8b..8b78618b4 100644 --- a/src/slave/containerizer/device_manager/device_manager.hpp +++ b/src/slave/containerizer/device_manager/device_manager.hpp @@ -68,6 +68,18 @@ public: { std::vector<cgroups::devices::Entry> allow_list; std::vector<cgroups::devices::Entry> deny_list; + bool normalized() const; + // A device access is granted if it is encompassed by an allow entry + // and does not have access overlaps with any deny entry. + bool is_access_granted(const cgroups::devices::Entry& entry) const; + static Try<CgroupDeviceAccess> create( + const std::vector<cgroups::devices::Entry>& allow_list, + const std::vector<cgroups::devices::Entry>& deny_list); + + private: + CgroupDeviceAccess( + const std::vector<cgroups::devices::Entry>& allow_list, + const std::vector<cgroups::devices::Entry>& deny_list); }; static Try<DeviceManager*> create(const Flags& flags); diff --git a/src/tests/containerizer/cgroups2_tests.cpp b/src/tests/containerizer/cgroups2_tests.cpp index 39f17460c..e01b290b0 100644 --- a/src/tests/containerizer/cgroups2_tests.cpp +++ b/src/tests/containerizer/cgroups2_tests.cpp @@ -749,6 +749,48 @@ INSTANTIATE_TEST_CASE_P( {os::DEV_NULL, O_RDWR}, }})); +TEST_F(Cgroups2Test, ROOT_CGROUPS2_ConfigureValidation) +{ + const string& cgroup = TEST_CGROUP; + // Error if there is empty accesses in any entry + devices::Entry empty_entry = CHECK_NOTERROR(devices::Entry::parse("c 1:3 w")); + empty_entry.access.read = false; + empty_entry.access.write = false; + empty_entry.access.mknod = false; + vector<devices::Entry> allow = {empty_entry}; + vector<devices::Entry> deny = { + CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}; + Try<Nothing> configure_status = devices::configure(cgroup, allow, deny); + EXPECT_ERROR(configure_status); + EXPECT_EQ( + string("Failed to validate arguments: allow or deny lists are not ") + + string("normalized."), + configure_status.error()); + // Error if there is any entry that shares the same type, major, and minor + // numbers with another entry in the same list + allow = { + CHECK_NOTERROR(devices::Entry::parse("b 3:1 rw")), + CHECK_NOTERROR(devices::Entry::parse("b 3:1 m"))}; + deny = {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}; + configure_status = devices::configure(cgroup, allow, deny); + EXPECT_ERROR(configure_status); + EXPECT_EQ( + string("Failed to validate arguments: allow or deny lists are not ") + + string("normalized."), + configure_status.error()); + // Error if there are entries which are encompassed by another on the same + // list + allow = { + CHECK_NOTERROR(devices::Entry::parse("c *:* rw")), + CHECK_NOTERROR(devices::Entry::parse("c 3:1 w"))}; + deny = {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}; + configure_status = devices::configure(cgroup, allow, deny); + EXPECT_ERROR(configure_status); + EXPECT_EQ( + string("Failed to validate arguments: allow or deny lists are not ") + + string("normalized."), + configure_status.error()); +} TEST_F(Cgroups2Test, ROOT_CGROUPS2_AtomicReplace) { diff --git a/src/tests/device_manager_tests.cpp b/src/tests/device_manager_tests.cpp index 54d464e97..999273c46 100644 --- a/src/tests/device_manager_tests.cpp +++ b/src/tests/device_manager_tests.cpp @@ -109,6 +109,101 @@ TEST(NonWildcardEntry, NonWildcardFromWildcard) } +TEST(CgroupDeviceAccessTest, IsAccessGrantedTest) +{ + // Character devices with major minor numbrs 1:3 can do write only: + DeviceManager::CgroupDeviceAccess cgroup_device_access = + CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( + {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}, {})); + EXPECT_TRUE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w"))); + EXPECT_FALSE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 rw"))); + EXPECT_FALSE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("b 1:3 w"))); + // Character devices with minor number 3 can do write only: + cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( + {CHECK_NOTERROR(devices::Entry::parse("c *:3 w"))}, {})); + EXPECT_TRUE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 4:3 w"))); + EXPECT_TRUE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c *:3 w"))); + // Character devices with major number 5 can do write only: + cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( + {CHECK_NOTERROR(devices::Entry::parse("c 5:* w"))}, {})); + EXPECT_TRUE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:* w"))); + EXPECT_TRUE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:2 w"))); + // All devices will match the catch-all and can perform all operations: + cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( + {CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))}, {})); + EXPECT_TRUE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 6:2 w"))); + // Deny all accesses to character device with major and numbers 1:3 + cgroup_device_access.deny_list = {*devices::Entry::parse("c 1:3 rwm")}; + cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( + {}, {CHECK_NOTERROR(devices::Entry::parse("c 1:3 rwm"))})); + EXPECT_FALSE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w"))); + EXPECT_FALSE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 rw"))); + + cgroup_device_access.allow_list = {}; + cgroup_device_access.deny_list = {}; + // Entry should be denied if encompassed by an entry in allow_list and + // overlaps with entry in deny_list + cgroup_device_access.allow_list.push_back(*devices::Entry::parse("c 1:3 rw")); + cgroup_device_access.deny_list.push_back(*devices::Entry::parse("c 1:3 w")); + EXPECT_FALSE( + cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 rw"))); +} + +TEST(CgroupDeviceAccessTest, NormalizedTest) +{ + // Not normalized if there is an entry with no accesses specified + devices::Entry empty_entry; + empty_entry.selector.type = devices::Entry::Selector::Type::CHARACTER; + empty_entry.selector.major = 1; + empty_entry.selector.minor = 3; + empty_entry.access.read = false; + empty_entry.access.write = false; + empty_entry.access.mknod = false; + Try<DeviceManager::CgroupDeviceAccess> cgroup_device_access = + DeviceManager::CgroupDeviceAccess::create( + {empty_entry}, {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}); + EXPECT_ERROR(cgroup_device_access); + EXPECT_EQ( + "Failed to create CgroupDeviceAccess: The allow or deny list is not " + "normalized.", + cgroup_device_access.error()); + + // Not normalized if there is any entry that shares the same type, major, and + // minor numbers with another entry in the same list + cgroup_device_access = DeviceManager::CgroupDeviceAccess::create( + {CHECK_NOTERROR(devices::Entry::parse("b 3:1 rw")), + CHECK_NOTERROR(devices::Entry::parse("b 3:1 m"))}, + {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}); + EXPECT_ERROR(cgroup_device_access); + EXPECT_EQ( + "Failed to create CgroupDeviceAccess: The allow or deny list is not " + "normalized.", + cgroup_device_access.error()); + + // Not normalized if there are entries which are encompassed by another on the + // same list + cgroup_device_access = DeviceManager::CgroupDeviceAccess::create( + {CHECK_NOTERROR(devices::Entry::parse("c *:* rw")), + CHECK_NOTERROR(devices::Entry::parse("c 3:1 w"))}, + {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}); + EXPECT_ERROR(cgroup_device_access); + EXPECT_EQ( + "Failed to create CgroupDeviceAccess: The allow or deny list is not " + "normalized.", + cgroup_device_access.error()); +} + + TEST_F(DeviceManagerTest, ROOT_DeviceManagerConfigure_Normal) { typedef std::pair<string, int> OpenArgs;
