From 81707abd33d2ddebcd8ceeb08dfc01bf86d8badd Mon Sep 17 00:00:00 2001 From: Vasiliy Ulyanov Date: Sun, 7 Feb 2021 19:03:18 +0100 Subject: [PATCH 1/2] ebpf: fix device access check Checking the access mode as bellow if (R3 & bpfAccess == 0 /* use R1 as a temp var */) goto next does not handle correctly device file probing with: access(dev_name, F_OK) F_OK does not trigger read or write access. Instead the access type in R3 in that case will be zero and the check will not pass even if "rw" is allowed for the device file. Comparing the 'masked' access type with the requested one solves the issue: if (R3 & bpfAccess != R3 /* use R1 as a temp var */) goto next Signed-off-by: Vasiliy Ulyanov --- libcontainer/cgroups/ebpf/devicefilter/devicefilter.go | 4 ++-- libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go index a173fd4a16f..fcd3746e023 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go +++ b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go @@ -127,10 +127,10 @@ func (p *program) appendDevice(dev *devices.Rule) error { } if hasAccess { p.insts = append(p.insts, - // if (R3 & bpfAccess == 0 /* use R1 as a temp var */) goto next + // if (R3 & bpfAccess != R3 /* use R1 as a temp var */) goto next asm.Mov.Reg32(asm.R1, asm.R3), asm.And.Imm32(asm.R1, bpfAccess), - asm.JEq.Imm(asm.R1, 0, nextBlockSym), + asm.JNE.Reg(asm.R1, asm.R3, nextBlockSym), ) } if hasMajor { diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go index f714bcac272..cfcaa20306a 100644 --- a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go +++ b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go @@ -121,7 +121,7 @@ block-9: 50: JNEImm dst: r2 off: -1 imm: 1 51: Mov32Reg dst: r1 src: r3 52: And32Imm dst: r1 imm: 1 - 53: JEqImm dst: r1 off: -1 imm: 0 + 53: JNEReg dst: r1 off: -1 src: r3 54: Mov32Imm dst: r0 imm: 1 55: Exit block-10: @@ -129,7 +129,7 @@ block-10: 56: JNEImm dst: r2 off: -1 imm: 2 57: Mov32Reg dst: r1 src: r3 58: And32Imm dst: r1 imm: 1 - 59: JEqImm dst: r1 off: -1 imm: 0 + 59: JNEReg dst: r1 off: -1 src: r3 60: Mov32Imm dst: r0 imm: 1 61: Exit block-11: From efb8552b05431520d66ecd970628b35126039629 Mon Sep 17 00:00:00 2001 From: Vasiliy Ulyanov Date: Mon, 8 Feb 2021 13:47:30 +0100 Subject: [PATCH 2/2] tests/int: add device access test The test verifies if the device file can be queried using 'access(dev_name, F_OK)' when the permissions are set to 'rw'. The call does not explicitly trigger read or write access but should succeed. Signed-off-by: Vasiliy Ulyanov --- tests/integration/dev.bats | 8 +++++++- tests/integration/testdata/dev_access_test.c | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 tests/integration/testdata/dev_access_test.c diff --git a/tests/integration/dev.bats b/tests/integration/dev.bats index 11c53c95532..5ef51552550 100644 --- a/tests/integration/dev.bats +++ b/tests/integration/dev.bats @@ -59,7 +59,7 @@ function teardown() { @test "runc run [device cgroup allow rw char device]" { requires root - update_config ' .linux.resources.devices = [{"allow": false, "access": "rwm"},{"allow": true, "type": "c", "major": 1, "minor": 11, "access": "rwm"}] + update_config ' .linux.resources.devices = [{"allow": false, "access": "rwm"},{"allow": true, "type": "c", "major": 1, "minor": 11, "access": "rw"}] | .linux.devices = [{"path": "/dev/kmsg", "type": "c", "major": 1, "minor": 11}] | .process.args |= ["sh"] | .hostname = "myhostname"' @@ -75,6 +75,12 @@ function teardown() { # test read runc exec test_allow_char sh -c 'head -n 1 /dev/kmsg' [ "$status" -eq 0 ] + + # test access + TEST_NAME="dev_access_test" + gcc -static -o "rootfs/bin/${TEST_NAME}" "${TESTDATA}/${TEST_NAME}.c" + runc exec test_allow_char sh -c "${TEST_NAME} /dev/kmsg" + [ "$status" -eq 0 ] } @test "runc run [device cgroup allow rm block device]" { diff --git a/tests/integration/testdata/dev_access_test.c b/tests/integration/testdata/dev_access_test.c new file mode 100644 index 00000000000..705096c6d01 --- /dev/null +++ b/tests/integration/testdata/dev_access_test.c @@ -0,0 +1,17 @@ +#include +#include + +int main(int argc, char *argv[]) +{ + const char *dev_name = "/dev/kmsg"; + + if (argc > 1) + dev_name = argv[1]; + + if (access(dev_name, F_OK) < 0) { + perror(dev_name); + return 1; + } + + return 0; +}