Repository: opencontainers/runc
Pull request #2796 (2021-02-07 18:53:21)

ebpf: fix device access check
I was trying to expose a host character device to a container on a system with `cgroup v2` enabled. Looks like there is an issue with  permissions checking in the device filtering `ebpf` program that is loaded and attached to the container's cgroup.

This PR fixes the issue when a call to 

    access(dev_name, F_OK)

from a container fails with `EPERM` error even though the permissions for the device file are set to `rw`. The problem is not reproducible with `rwm` as in this case the check for permissions is skipped. Similar solution is used in systemd:

https://github.com/systemd/systemd/blob/8e45c72cf581a2224725469376d13ca4dcd77a74/src/core/bpf-devices.c#L145

I built and verified locally the proposed solution and also fixed the unit-tests accordingly. Hope the fix might be usefull.

**Description**

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



Patch:
diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter.go
index a173fd4a..fcd3746e 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 {


Test patch:
diff --git a/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go b/libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go
index f714bcac..cfcaa203 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 <block-10>
         51: Mov32Reg dst: r1 src: r3
         52: And32Imm dst: r1 imm: 1
-        53: JEqImm dst: r1 off: -1 imm: 0 <block-10>
+        53: JNEReg dst: r1 off: -1 src: r3 <block-10>
         54: Mov32Imm dst: r0 imm: 1
         55: Exit
 block-10:
@@ -129,7 +129,7 @@ block-10:
         56: JNEImm dst: r2 off: -1 imm: 2 <block-11>
         57: Mov32Reg dst: r1 src: r3
         58: And32Imm dst: r1 imm: 1
-        59: JEqImm dst: r1 off: -1 imm: 0 <block-11>
+        59: JNEReg dst: r1 off: -1 src: r3 <block-11>
         60: Mov32Imm dst: r0 imm: 1
         61: Exit
 block-11:
diff --git a/tests/integration/dev.bats b/tests/integration/dev.bats
index 11c53c95..5ef51552 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 00000000..705096c6
--- /dev/null
+++ b/tests/integration/testdata/dev_access_test.c
@@ -0,0 +1,17 @@
+#include <stdio.h>
+#include <unistd.h>
+
+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;
+}