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
The following commit(s) were added to refs/heads/master by this push:
new acb5a8a50 [ebpf] Correct ebpf deny block behavior.
acb5a8a50 is described below
commit acb5a8a50cc285146726ce5518fd42499e373f87
Author: Jason Zhou <[email protected]>
AuthorDate: Thu Jul 25 19:52:55 2024 -0400
[ebpf] Correct ebpf deny block behavior.
Currently, the deny block matches a device access iff all accesses
match on the deny block. For example, a rw access would not match the
deny block even if the deny block had w access specified.
We would expect that the deny block should deny all accesses if the
type, major, and minor number matches, and if any of the device accesses
overlap with what's specified in the deny block.
Review: https://reviews.apache.org/r/75109/
---
src/linux/cgroups2.cpp | 46 ++++-
src/tests/containerizer/cgroups2_tests.cpp | 272 ++++++++++++++---------------
2 files changed, 168 insertions(+), 150 deletions(-)
diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp
index cb3c425a4..ac4678261 100644
--- a/src/linux/cgroups2.cpp
+++ b/src/linux/cgroups2.cpp
@@ -1250,8 +1250,8 @@ public:
short allow_block_trailer_size = allow_block_trailer(0).size();
foreach (const Entry& entry, allow) {
- vector<bpf_insn> allow_block =
- add_device_checks(entry, allow_block_trailer_size);
+ vector<bpf_insn> allow_block = add_device_checks(
+ entry, allow_block_trailer_size, DeviceCheckType::ALLOW);
allow_device_check_blocks.push_back(allow_block);
start_of_deny_jmp_size += allow_block.size() +
allow_block_trailer_size;
@@ -1273,7 +1273,8 @@ public:
// We are either following the normal code flow or special case 1 (see
// diagram above) if we reached this section.
foreach (const Entry& entry, deny) {
- program.append(add_device_checks(entry, deny_block_trailer().size()));
+ program.append(add_device_checks(
+ entry, deny_block_trailer().size(), DeviceCheckType::DENY));
program.append(deny_block_trailer());
}
@@ -1289,9 +1290,16 @@ public:
}
private:
+ enum DeviceCheckType
+ {
+ ALLOW,
+ DENY
+ };
+
static vector<bpf_insn> add_device_checks(
const Entry& entry,
- short trailer_length)
+ short trailer_length,
+ DeviceCheckType device_check_type)
{
// We create a block of bytecode with the format:
// 1. Major Version Check
@@ -1324,7 +1332,25 @@ private:
});
};
- auto check_access_instructions =
+ auto check_deny_access_instructions =
+ [](short jmp_size, const Entry::Access& access)
+ {
+ int bpf_access = 0;
+ bpf_access |= access.read ? BPF_DEVCG_ACC_READ : 0;
+ bpf_access |= access.write ? BPF_DEVCG_ACC_WRITE : 0;
+ bpf_access |= access.mknod ? BPF_DEVCG_ACC_MKNOD : 0;
+ return vector<bpf_insn>({
+ BPF_MOV32_REG(BPF_REG_1, BPF_REG_3),
+ BPF_ALU32_IMM(BPF_AND, BPF_REG_1, bpf_access),
+ BPF_JMP_IMM(
+ BPF_JEQ,
+ BPF_REG_1,
+ 0,
+ static_cast<short>(jmp_size - 2)),
+ });
+ };
+
+ auto check_allow_access_instructions =
[](short jmp_size, const Entry::Access& access)
{
int bpf_access = 0;
@@ -1368,10 +1394,14 @@ private:
// The jump sizes here correspond to the size of the bpf instructions
// that each check adds to the program. The total size of the block is
// the trailer length plus the total length of all checks.
+ size_t access_insn_size =
+ device_check_type == DeviceCheckType::ALLOW
+ ? check_allow_access_instructions(0, access).size()
+ : check_deny_access_instructions(0, access).size();
short nxt_blk_jmp_size = trailer_length
+ (check_major ? check_major_instructions(0, 0).size() : 0)
+ (check_minor ? check_minor_instructions(0, 0).size() : 0)
- + (check_access ? check_access_instructions(0, access).size() : 0)
+ + (check_access ? access_insn_size : 0)
+ (check_type ? check_type_instructions(0, selector).size() : 0);
// We subtract one because the program counter will be one ahead when it
@@ -1414,7 +1444,9 @@ private:
// 4. Check access (r3) against entry.
if (check_access) {
vector<bpf_insn> insert_instructions =
- check_access_instructions(nxt_blk_jmp_size, access);
+ device_check_type == DeviceCheckType::ALLOW
+ ? check_allow_access_instructions(nxt_blk_jmp_size, access)
+ : check_deny_access_instructions(nxt_blk_jmp_size, access);
foreach (const bpf_insn& insn, insert_instructions) {
device_check_block.push_back(insn);
}
diff --git a/src/tests/containerizer/cgroups2_tests.cpp
b/src/tests/containerizer/cgroups2_tests.cpp
index c73045632..39f17460c 100644
--- a/src/tests/containerizer/cgroups2_tests.cpp
+++ b/src/tests/containerizer/cgroups2_tests.cpp
@@ -624,158 +624,144 @@ TEST_P(DeviceControllerTestFixture,
ROOT_CGROUPS2_DeviceController)
INSTANTIATE_TEST_CASE_P(
- DeviceControllerTestParams,
- DeviceControllerTestFixture,
- ::testing::Values<DeviceControllerTestParams>(
- // Acceses are blocked by default if no entries are configured.
- DeviceControllerTestParams{
- vector<devices::Entry>{},
- vector<devices::Entry>{},
- vector<OpenArgs>{},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDWR}}
- },
- // Deny access to /dev/null.
- DeviceControllerTestParams{
- vector<devices::Entry>{},
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
- vector<OpenArgs>{},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}
- },
- // Allow access to /dev/null.
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
- vector<devices::Entry>{},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}},
- vector<OpenArgs>{}
- },
- // Allow read-only access to /dev/null.
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 r")},
- vector<devices::Entry>{},
- vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}
- },
- // Allow write-only access to /dev/null.
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
- vector<devices::Entry>{},
- // write-only allowed
- vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}},
- // read is blocked
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
- },
- // Do not allow device if it's on both allow and deny list
- DeviceControllerTestParams{
- vector<devices::Entry>{
- *devices::Entry::parse("c 1:3 w"),
- *devices::Entry::parse("b 1:3 w")},
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
- vector<OpenArgs>{},
- // write-only is blocked
- vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}
- },
- // Mismatched entry in deny list is ignored and write access is granted
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
- vector<devices::Entry>{*devices::Entry::parse("b 1:3 w")},
- // write-only allowed
- vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}},
- // read is blocked
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
- },
- // Access to /dev/null is denied because the allowed access is to
- // a different device type with the same major:minor numbers.
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("b 1:3 rwm")},
- vector<devices::Entry>{},
- vector<OpenArgs>{},
- // /dev/null is blocked
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
- },
- // Allow access to all devices.
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("a *:* rwm")},
- vector<devices::Entry>{},
- vector<OpenArgs>{
- {os::DEV_NULL, O_WRONLY},
- {os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}},
- vector<OpenArgs>{},
- },
- // Allow access with catch-all and specified device
- DeviceControllerTestParams{
- vector<devices::Entry>{
- *devices::Entry::parse("a *:* rwm"),
- *devices::Entry::parse("c 1:3 w")},
- vector<devices::Entry>{},
- vector<OpenArgs>{
- {os::DEV_NULL, O_WRONLY},
- {os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}},
- vector<OpenArgs>{},
- },
- // Allow access to all devices except one
- DeviceControllerTestParams{
- vector<devices::Entry>{*devices::Entry::parse("a *:* rwm")},
- vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
- // read is allowed by catch-all
- vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
- // write-only is blocked
- vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}
- },
- // Deny access to all devices
- DeviceControllerTestParams{
- vector<devices::Entry>{},
- vector<devices::Entry>{*devices::Entry::parse("a *:* rwm")},
- vector<OpenArgs>{},
- vector<OpenArgs>{
- {os::DEV_NULL, O_WRONLY},
- {os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}},
- },
- // Deny access using catch-all and specified device
- DeviceControllerTestParams{
- vector<devices::Entry>{},
- vector<devices::Entry>{
- *devices::Entry::parse("c 1:3 w"),
- *devices::Entry::parse("a *:* rwm")},
- vector<OpenArgs>{},
- vector<OpenArgs>{
- {os::DEV_NULL, O_WRONLY},
- {os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}},
- },
- // Catch-all in both allow and deny list
- DeviceControllerTestParams{
- vector<devices::Entry>{
- *devices::Entry::parse("c 1:3 w"),
- *devices::Entry::parse("a *:* rwm")},
- vector<devices::Entry>{*devices::Entry::parse("a *:* rwm")},
- vector<OpenArgs>{},
- vector<OpenArgs>{
- {os::DEV_NULL, O_WRONLY},
- {os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}},
- }
- ));
+ DeviceControllerTestParams,
+ DeviceControllerTestFixture,
+ ::testing::Values<DeviceControllerTestParams>(
+ // Acceses are blocked by default if no entries are configured.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{},
+ vector<devices::Entry>{},
+ vector<OpenArgs>{},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDWR}}
+ },
+ // Deny access to /dev/null.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{},
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("c 1:3 rwm"))},
+ vector<OpenArgs>{},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}
+ },
+ // Allow access to /dev/null.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("c 1:3 rwm"))},
+ vector<devices::Entry>{},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}},
+ vector<OpenArgs>{}
+ },
+ // Allow read-only access to /dev/null.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("c 1:3 r"))},
+ vector<devices::Entry>{},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}
+ },
+ // Allow write-only access to /dev/null.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))},
+ vector<devices::Entry>{},
+ // Write-only allowed.
+ vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}},
+ // Read is blocked.
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
+ // Do not allow device if it's on both allow and deny list.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("c 1:3 w")),
+ CHECK_NOTERROR(devices::Entry::parse("b 1:3 w"))},
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))},
+ vector<OpenArgs>{},
+ // Write-only is blocked.
+ vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}
+ },
+ // Mismatched entry in deny list is ignored and write access is granted.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))},
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("b 1:3 w"))},
+ // Write-only allowed.
+ vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}},
+ // Read is blocked.
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
+ // Access to /dev/null is denied because the allowed access is to
+ // a different device type with the same major:minor numbers.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("b 1:3 rwm"))},
+ vector<devices::Entry>{},
+ vector<OpenArgs>{},
+ // /dev/null is blocked.
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
+ // Allow access to all devices.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))},
+ vector<devices::Entry>{},
+ vector<OpenArgs>{
+ {os::DEV_NULL, O_WRONLY},
+ {os::DEV_NULL, O_RDWR},
+ {os::DEV_NULL, O_RDONLY}},
+ vector<OpenArgs>{}
+ },
+ // Allow access to all devices except one.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))},
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))},
+ // Read is allowed by catch-all.
+ vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
+ // Write-only is blocked.
+ vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}
+ },
+ // Deny access to all devices.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{},
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))},
+ vector<OpenArgs>{},
+ vector<OpenArgs>{
+ {os::DEV_NULL, O_WRONLY},
+ {os::DEV_NULL, O_RDWR},
+ {os::DEV_NULL, O_RDONLY}}
+ },
+ // Catch-all in both allow and deny list.
+ DeviceControllerTestParams{
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))},
+ vector<devices::Entry>{
+ CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))},
+ vector<OpenArgs>{},
+ vector<OpenArgs>{
+ {os::DEV_NULL, O_WRONLY},
+ {os::DEV_NULL, O_RDWR},
+ {os::DEV_NULL, O_RDONLY}}
+ },
+ // Deny all accesses involving write
+ DeviceControllerTestParams{
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("a *:*
rw"))},
+ vector<devices::Entry>{CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
+ vector<OpenArgs>{
+ {os::DEV_NULL, O_WRONLY},
+ {os::DEV_NULL, O_RDWR},
+ }}));
TEST_F(Cgroups2Test, ROOT_CGROUPS2_AtomicReplace)
{
const string& cgroup = TEST_CGROUP;
- const vector<devices::Entry>& allow = {*devices::Entry::parse("c 1:3 w")};
+ const vector<devices::Entry>& allow = {
+ CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))};
const vector<devices::Entry>& deny = {};
- const vector<OpenArgs>& allowed_accesses = {
- {os::DEV_NULL, O_WRONLY}
- };
+ const vector<OpenArgs>& allowed_accesses = {{os::DEV_NULL, O_WRONLY}};
const vector<OpenArgs>& blocked_accesses = {
- {os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}
- };
+ {os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}};
const vector<devices::Entry>& to_be_replaced_allow = {};
const vector<devices::Entry>& to_be_replaced_deny = {
- *devices::Entry::parse("c 1:3 w")
- };
+ CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))};
ASSERT_SOME(cgroups2::create(cgroup));
string path = cgroups2::path(cgroup);
