ebpf: fix device access check#2796
Conversation
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 <vulyanov@suse.de>
|
Can we have an integration test in dev.bats ? |
|
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 |
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 <vulyanov@suse.de>
bc28eff to
efb8552
Compare
|
@AkihiroSuda , I added the test. Unfortunately I was not able to find some busybox tool that uses @cyphar ,
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. |
|
I'll work on the emulator patch myself, this patch is okay for now since it fixes a separate issue with the program generation. |
| # 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" |
There was a problem hiding this comment.
probably no need to wrap this into sh -c, iow
| runc exec test_allow_char sh -c "${TEST_NAME} /dev/kmsg" | |
| runc exec test_allow_char "${TEST_NAME}" /dev/kmsg |
will work just fine.
There was a problem hiding this comment.
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.
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 |
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. |
I was trying to expose a host character device to a container on a system with
cgroup v2enabled. Looks like there is an issue with permissions checking in the device filteringebpfprogram that is loaded and attached to the container's cgroup.This PR fixes the issue when a call to
from a container fails with
EPERMerror even though the permissions for the device file are set torw. The problem is not reproducible withrwmas 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
does not handle correctly device file probing with:
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
rwis allowed for the device file. Comparing the 'masked' access type with the requested one solves the issue: