Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ebpf: fix device access check #2796

Merged
merged 2 commits into from
Feb 10, 2021
Merged

ebpf: fix device access check #2796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81707ab
ebpf: fix device access check
vasiliy-ul Feb 7, 2021
efb8552
tests/int: add device access test
vasiliy-ul Feb 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libcontainer/cgroups/ebpf/devicefilter/devicefilter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/cgroups/ebpf/devicefilter/devicefilter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,15 @@ 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:
// (c, wildcard, wildcard, m, true)
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:
Expand Down
8 changes: 7 additions & 1 deletion tests/integration/dev.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"'
Expand All @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important though

Copy link
Contributor Author

@vasiliy-ul vasiliy-ul Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[ "$status" -eq 0 ]
}

@test "runc run [device cgroup allow rm block device]" {
Expand Down
17 changes: 17 additions & 0 deletions tests/integration/testdata/dev_access_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}