ebpf: fix device access check

[vasiliy-ul]

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

[AkihiroSuda]

Can we have an integration test in dev.bats ?

[cyphar]

I think the wildcard behaviour is also incorrect (it ignores all entries after a wildcard). I think it would make far more sense to use the emulator to apply all of the rules and then create a program based on the minimal ruleset.

[vasiliy-ul]

@AkihiroSuda , I added the test. Unfortunately I was not able to find some busybox tool that uses access syscall. Also the shell file tests (e.g. [ -e /dev/kmsg ]) are based on stat which cannot be used to reproduce the issue. Therefore I added a small C program that is built during testing.

@cyphar ,

I think the wildcard behaviour is also incorrect (it ignores all entries after a wildcard). I think it would make far more sense to use the emulator to apply all of the rules and then create a program based on the minimal ruleset.

I haven't not tried to apply a wildcard rule. In my case I have specific devices to allow. Though I think this fix should be valid also for the minimal rule set.

[cyphar]

I'll work on the emulator patch myself, this patch is okay for now since it fixes a separate issue with the program generation.

[cyphar]

LGTM.

[kolyshkin]

probably no need to wrap this into sh -c, iow

Suggested change

	runc exec test_allow_char sh -c "${TEST_NAME} /dev/kmsg"
	runc exec test_allow_char "${TEST_NAME}" /dev/kmsg

will work just fine.

[kolyshkin]

Not important though

[vasiliy-ul]

Agree. I just followed the same approach as in other tests here. So used sh -c rather for consistency across this file. I would rather keep it just to have a common style.

[kolyshkin]

LGTM

[kolyshkin]

Unfortunately I was not able to find some busybox tool that uses access syscall.

You can actually use Debian Buster instead of Busybox for any test file, but I'm not sure you can find a simple binary that uses access(2) in there either.

[vasiliy-ul]

Unfortunately I was not able to find some busybox tool that uses access syscall.

You can actually use Debian Buster instead of Busybox for any test file, but I'm not sure you can find a simple binary that uses access(2) in there either.

Yeah, I spent some time on strace'ing different tools trying to find such binary but eventually writing a small helper turned out to be the easiest solution.