Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snap-seccomp: add in-kernel bpf tests #3502
Conversation
Some checks were not successful
codecov-io
commented
Jun 21, 2017
•
Edited 1 time
-
codecov-io
Jun 21, 2017
-
Jun 21, 2017
Codecov Report
@@ Coverage Diff @@
## master #3502 +/- ##
==========================================
+ Coverage 75.7% 75.72% +0.01%
==========================================
Files 409 409
Lines 35227 35227
==========================================
+ Hits 26670 26677 +7
+ Misses 6672 6667 -5
+ Partials 1885 1883 -2
Continue to review full report at Codecov.
|
mvo5
added some commits
Jun 21, 2017
Some checks were not successful
mvo5
referenced this pull request
Jun 21, 2017
Merged
interfaces: simplify snap-confine by just loading pre-generated bpf code #3431
mvo5
added some commits
Jun 21, 2017
Some checks were not successful
All checks have passed
All checks have passed
| +int main(int argc, char **argv) { | ||
| + int l[7]; | ||
| + for (int i=0; i<7;i++) | ||
| + l[i] = atoi(argv[i+1]); |
jdstrand
Jun 23, 2017
Contributor
strtol() is best practice, but as test code this is ok since we control all the input.
| + return err.Error() | ||
| + } | ||
| + l := strings.Split(string(output), "\n") | ||
| + return l[len(l)-2] |
jdstrand
Jun 23, 2017
Contributor
This is racy and trying to pick a needle out of a potential kernel logging haystack. It might be better to use lastKmsg() only when you have an unexpected error and return something like:
Showing last 10 lines of dmesg:
show
your
last
10
lines
of
dmesg
output
right
here
| + cmd.Stderr = os.Stderr | ||
| + err = cmd.Run() | ||
| + c.Assert(err, IsNil) | ||
| + |
| +sysinfo | ||
| +exit | ||
| +# i386 | ||
| +set_thread_area |
jdstrand
Jun 23, 2017
Contributor
You might need other on other architectures. If so, these can of easily be identified in the spread tests.
zyga
Aug 16, 2017
Contributor
Yeah, I was about to write that this list is a great way to learn how linux userspace works across architectures. I don't mind having it but I bet this will bite us the first time this runs on s360 and you have to fix the tests.
jdstrand
Aug 30, 2017
Contributor
That's ok, it's easy to fix. Note that this list is not very different from what was in the old snap-confine unit tests.
| + if strings.Contains(bpfInput, "PR_SET_ENDIAN") { | ||
| + c.Logf("cannot run PR_SET_ENDIAN in runBpfInKernel, this actually switches the endianess and the program crashes") | ||
| + return | ||
| + } |
jdstrand
Jun 23, 2017
Contributor
Looking at man syscall, it seems we may also want to skip on the following syscalls: fadvise64_64, ftruncate64, posix_fadvise, pread64, pwrite64, readahead, sync_file_range, and truncate64. Please add a comment here that we just skip syscalls with architecture-specific details and in seccompSyscallRunnerContent (right above the call to syscall()) indicating that there might be architecture-specific requirements and to see man syscall for details.
| - // https://github.com/golang/go/issues/20556 | ||
| - // switch to x/net/bpf once we can simulate seccomp bpf there | ||
| - bpf.VmEndianness = nativeEndian() | ||
| + s.runBpfInKernel(c, seccompWhitelist, bpfInput, expected) |
jdstrand
Jun 23, 2017
Contributor
Now that you added this, can you adjust the comment above simulateBpf to have:
// simulateBpf first:
// 1. runs main.Compile() which will catch syntax errors and output to a file
// 2. takes the output file from main.Compile and loads it via
// decodeBpfFromFile
// 3. parses the decoded bpf using the seccomp library and various
// snapd functions
// 4. runs the parsed bpf through a bpf VM
//
// Then simulateBpf runs the policy through the kernel by calling
// runBpfInKernel() which:
// 1. runs main.Compile()
// 2. the program in seccompBpfLoaderContent with the output file as an
// argument
// 3. the program in seccompBpfLoaderContent loads the output file BPF into
// the kernel and executes the program in seccompBpfRunnerContent with the
// syscall and arguments specified by the test
//
// In this manner, in addition to verifying policy syntax we are able to
// unit test the resulting bpf in several ways.
//
// Full testing of applied policy is done elsewhere via spread tests.
zyga
and others
added some commits
Jul 18, 2017
All checks have failed
Some checks were not successful
|
Thanks @jdstrand ! I addressed all comments now. |
pedronis
requested a review
from
zyga
Aug 3, 2017
zyga
reviewed
Aug 16, 2017
A few comments, I probably missed the reason for some changes as this was done so long ago and some context is lost in my mind.
| + s.seccompSyscallRunner = filepath.Join(c.MkDir(), "seccomp_syscall_runner") | ||
| + err = ioutil.WriteFile(s.seccompSyscallRunner+".c", seccompSyscallRunnerContent, 0644) | ||
| + c.Assert(err, IsNil) | ||
| + cmd = exec.Command("gcc", "-Werror", "-Wall", "-static", s.seccompSyscallRunner+".c", "-o", s.seccompSyscallRunner, "-Wl,-static", "-static-libgcc") |
zyga
Aug 16, 2017
Contributor
Does the -static matter here? I'm worried about another place that does static linking across the distro landscape.
mvo5
Aug 22, 2017
Collaborator
Yes, it explodes with: /usr/bin/ld: /usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/libc.a(libc-start.o): relocation R_X86_64_32 against undefined symbol _dl_starting_up' can not be used when making a shared object; recompile with -fPIC without that.
zyga
Aug 30, 2017
Contributor
Aha, I don't know why this happens but it will be another thing that needs distro-patching as static linking to libseccomp is not a given. I'll investigate and see if I can make this nicer.
| +sysinfo | ||
| +exit | ||
| +# i386 | ||
| +set_thread_area |
jdstrand
Jun 23, 2017
Contributor
You might need other on other architectures. If so, these can of easily be identified in the spread tests.
zyga
Aug 16, 2017
Contributor
Yeah, I was about to write that this list is a great way to learn how linux userspace works across architectures. I don't mind having it but I bet this will bite us the first time this runs on s360 and you have to fix the tests.
jdstrand
Aug 30, 2017
Contributor
That's ok, it's easy to fix. Note that this list is not very different from what was in the old snap-confine unit tests.
| + // Note that we will need to also skip: fadvise64_64, | ||
| + // ftruncate64, posix_fadvise, pread64, pwrite64, readahead, | ||
| + // sync_file_range, and truncate64. | ||
| + // Once we start using those. See `man syscall` |
zyga
Aug 16, 2017
Contributor
The comment and the next code block seem unrelated. Am I missing anything?
mvo5
Aug 22, 2017
Collaborator
I updated it a bit to clarify it some more. It is essentially about that we cannot test some syscalls using the in-kernel VM because when we run them things explodes.
| + args := strings.Split(l[2], ",") | ||
| + for i := range args { | ||
| + // init with random number argument | ||
| + syscallArg := (uint64)(rand.Uint32()) |
mvo5
Aug 22, 2017
Collaborator
Mostly to avoid that everything is set to "0" and we miss issues because we did not test other values.
| @@ -233,7 +444,7 @@ func (s *snapSeccompSuite) TestCompile(c *C) { | ||
| {"read\nwrite\nexecve\n", "write", main.SeccompRetAllow}, | ||
| // trivial denial | ||
| - {"read", "execve", main.SeccompRetKill}, | ||
| + {"read", "ioctl", main.SeccompRetKill}, |
mvo5
Aug 22, 2017
Collaborator
Execve is now part of the common syscalls, i.e. it must be allowed so that we can actually run these tests here.
| // bad input | ||
| - for _, bad := range []string{"quotactl;native;99999", "read;native;"} { |
mvo5
added some commits
Aug 22, 2017
All checks have passed
niemeyer
reviewed
Aug 29, 2017
| +#include<sys/syscall.h> | ||
| +#include<stdlib.h> | ||
| +int main(int argc, char **argv) { | ||
| + int l[7]; |
niemeyer
Aug 29, 2017
Contributor
The indentation here is different from the one above. We have three spaces here and a tab above, apparently.
|
@zyga This is ready for another pass. |
zyga
and others
added some commits
Aug 30, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
Some checks were not successful
|
FYI, the changes since I approved this look fine (though I'm not sure we we chose 'webkit' for clang formatting, but not a blocker). |
|
@jdstrand I'm equally puzzled about the webkit formating but I agree its not a blocker. |
|
@jdstrand I'm equally puzzled about the webkit formating but I agree its not a blocker. |
|
@jdstrand Can you please mark as an actual approval so it's more clear in the PR status? |
mvo5
changed the title from
snap-seccomp: add more tests
to
snap-seccomp: add in-kernel bpf tests
Aug 30, 2017
Some checks were not successful
mvo5
added some commits
Aug 30, 2017
All checks have passed
Some checks were not successful
mvo5 commentedJun 21, 2017
Based on #3431 - this adds extra tests by generating test binaries and run them against the in-kernel bpf VM.