Fix breaking change in Seccomp profile behavior #1616
Conversation
|
@mrunalp PTAL, this should fix the issue identified by the CRI-O tests |
mheon
force-pushed
the
seccomp_fix_breakage
branch
from
7ea194b to
d54d5aa
Compare
| if err = filter.AddRuleConditional(callNum, callAct, conditions); err != nil { | ||
| return err | ||
| hasMultipleArgs := false | ||
| for _, count := range argCounts { |
There was a problem hiding this comment.
nit, I might be missing a golang nuance, but could you just do "len(argCounts) > 1"?
There was a problem hiding this comment.
len(argCounts) should always be 6, as it's a 6-long array of uints, one for each argument possible in a Linux sycall. Each one holds the number of conditions that check that specific argument. If we had reduce in Go, I could make this a one-liner, but as things are needs to check each individual field in the array so it needs to iterate.
There was a problem hiding this comment.
Ah, I see the 6 now at line 184. Missed that, was thinking this was dynamically allocated. Thanks for the 411.
|
LGTM assuming happy test. One minor question. Also do we have a trello card or issue created to go back and get to the correct behaviour at some point in time? |
|
@TomSweeneyRedHat This seems like a user configuration issue from the |
| // Conditional match - convert the per-arg rules into library format | ||
| // If two or more arguments have the same condition, | ||
| // Revert to old behavior, adding each condition as a separate rule | ||
| argCounts := make([]uint, 6) |
There was a problem hiding this comment.
While I recognise that currently Linux only has 6-argument syscalls, and it's unlikely we'll ever have 7-argument syscalls, can we make this a global MaxSyscallArguments somewhere?
There was a problem hiding this comment.
Should this constant live in runc or the libseccomp bindings? I think the bindings would be the logical place, but it would require revendoring once the change lands.
There was a problem hiding this comment.
I agree with putting it in the libseccomp bindings, but let's keep in runc for now until the change lands -- we can do that separately to this change.
| conditions := []libseccomp.ScmpCondition{} | ||
|
|
||
| for _, cond := range call.Args { | ||
| argCounts[cond.Index] += 1 |
There was a problem hiding this comment.
And I'm worried this will panic if someone gives an invalid index as a configuration. We should error out here if cond.Index >= MaxSyscallArguments.
There was a problem hiding this comment.
I think we can actually just move the line down below the logic that creates a Libseccomp condition, which has logic to catch invalid indexes. Should resolve the issue without introducing additional checks.
Multiple conditions were previously allowed to be placed upon the same syscall argument. Restore this behavior. Signed-off-by: Matthew Heon <mheon@redhat.com>
mheon
force-pushed
the
seccomp_fix_breakage
branch
from
d54d5aa to
e9193ba
Compare
|
@cyphar Made requested changes. Can you check the constant to see if it's what you were looking for? |
|
Yup, that looks good. LGTM (the test failure is due to Travis' repos being broken at the moment -- I 'll re-trigger the tests later). |
|
LGTM |
5 participants
Multiple conditions were previously allowed to be placed upon the same syscall argument. My earlier patch #1424 fixed an issue in the Libseccomp bindings with conditional syscall matching, but this broke multiple conditions being placed upon the same argument. This patch restores the old behavior for rules which would be broken by the prior change.
Note that the prior behavior here is still not correct behavior - the conditions for these rules will still be logical-OR instead of logical-AND as they should be. I looked into generating rules that would act correctly, but that proved to be unreasonably complex in some cases.
I haven't added a warning for the badly-behaved profiles, though I am open to doing so.