On Thu, Aug 10, 2017 at 3:24 PM, Tyler Hicks <tyhi...@canonical.com> wrote:
> On 08/10/2017 01:56 PM, Paul Moore wrote:
>> On Thu, Aug 10, 2017 at 2:24 PM, Tyler Hicks <tyhi...@canonical.com> wrote:
>>> Hello - I'm working on some libseccomp patches to support new kernel
>>> filter flags (SECCOMP_FILTER_FLAG_LOG and maybe
>>> SECCOMP_FILTER_FLAG_KILL_PROCESS) and return actions (SECCOMP_RET_LOG)
>>> being discussed upstream. I've bumped into an issue with the libseccomp
>>> test suite and would like to get some direction on how to proceed.
>>
>> Hi Tyler,
>
> Hey Paul!
>
>>
>> I must apologize, I've seen your patches upstream but I haven't had a
>> chance to give them any sort of review.
>
> No worries. I wasn't trying to drum up a review from you. :)

Okay, I feel less bad now :)

>>> The problems stem from the (new) need for libseccomp to call the
>>> seccomp() syscall in order to verify that the kernel supports the new
>>> filter flags and new return action. The seccomp() syscall can already be
>>> used to verify that specific filter flags are supported and will likely
>>> soon get a new operation that allows the caller to check if a specific
>>> action is supported.
>>>
>>> The first problem is that the build tests may be running under an older
>>> kernel that doesn't support the new features. If specifying the new
>>> SECCOMP_RET_LOG as an action, seccomp_rule_add() could fail due to the
>>> kernel not supporting the action and there's no way in the current test
>>> infrastructure to handle that. Additionally, seccomp_attr_set() may fail
>>> when trying to set one of the new filter flags.
>>
>> The good news is that we already have some provisions for runtime
>> checking of kernel capabilities, specifically the seccomp() syscall;
>> see src/system.c, sys_chk_seccomp_syscall() and
>> sys_chk_seccomp_flag().  I would need to better understand exactly how
>> the detection would work with your new patches, but I expect you could
>> do something similar.
>
> I'm aware of sys_chk_seccomp_syscall() and I'm building on top of it.
> Unfortunately, it doesn't address the test issues that I'm describing.
>
> I suspect that tests/13-basic-attrs.c would fail today if ran on a
> kernel that doesn't have the seccomp() syscall:
>
>         rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1);
>         if (rc != 0 && rc != -EOPNOTSUPP)
>                 goto out;
>         rc = seccomp_attr_get(ctx, SCMP_FLTATR_CTL_TSYNC, &val);
>         if (rc != 0)
>                 goto out;
>         if (val != 1) {
>                 rc = -1;
>                 goto out;
>         }
>
> The seccomp_attr_set() would fail with EOPNOTSUPP so the test would
> continue but then seccomp_attr_get() will return with val set to 0.

True, the 13-basic-attrs.c test would likely fail on a non-seccomp()
system as val would always be zero, but that really isn't too much of
a problem as we can treat that as a "known failure" when running the
tests on older systems.  The important part is that seccomp_attr_set()
and seccomp_attr_get() work correctly on both new and old systems, and
they do.  The problem isn't with the libseccomp library code, but
rather the test.

> This isn't a problem in practice because everyone is building
> libseccomp() on a kernel with seccomp(2) because that syscall has been
> around for a while.

... and for those few who are using an older system, the "known
failure" route is a reasonable one.  They may even patch around the
test.

> It will be a problem in practice once new filter flags land (in the
> kernel and libseccomp) and someone tries building libseccomp on a kernel
> that doesn't yet support them.

The #1 priority is to make sure that the library API works correctly
(see my comments above about seccomp_attr_{get,set}(...)), the #2
priority is to make the test work on both old and new systems.  If we
can't do #1, then we will not ever be able to use the new
functionality in libseccomp.

>>> The second problem is with the valgrind tests. Valgrind doesn't wrap
>>> seccomp(2):
>>>
>>>   https://bugs.kde.org/show_bug.cgi?id=345414
>>>   https://bugs.kde.org/show_bug.cgi?id=380183
>>>
>>> This means that the valgrind tests will always fail because libseccomp
>>> will see ENOSYS when attempting to verify that the kernel supports those
>>> new filter flags and the new action.
>>
>> At present libseccomp does not run valgrind on the "live" tests, only
>> the simulated BPF tests so this shouldn't be an issue.  If it was, we
>> would be seeing failures with the current (and past) releases.
>
> Currently libseccomp only changes behavior based on the presence of the
> seccomp() syscall when setting the TSYNC attribute and when loading a
> filter. As you said, valgrind doesn't run on the live tests so the
> latter isn't an issue.
>
> I think we'd see a failure in tests/13-basic-attrs.tests if valgrind was
> enabled for that test (only the "basic" test type is specified).

We don't run any valgrind tests on 13-basic-attrs, so it isn't worth
worrying about.

>>> The best solution that I can think of is for libseccomp to call
>>> secure_getenv(), prior to calling seccomp() to check feature support,
>>> and always blindly assume that a feature is supported if a "magic"
>>> environment variable is set. The test runner would set that env variable
>>> prior to running each test. Is this an acceptable solution? If not, do
>>> you have any ideas that you like better?
>>
>> My initial reaction to using environment variables in this manner is
>> not positive.  Let's try to do runtime checking in a similar manner as
>> we are doing now (see above comments) and if that doesn't work we can
>> reevaluate things, sound good?
>
> I don't like the env variable approach either, which is why I wanted to
> bring it up early.
>
> I have a slightly out of date (local) git branch that adds support for
> SECCOMP_FILTER_FLAG_LOG and SECCOMP_RET_LOG, using runtime checking that
> you're suggesting, that runs into the test failures that I initially
> described in this email thread.
>
> Note that I'm fine with delaying this discussion until the kernel
> patches are accepted and the libseccomp PR is proposed but I wanted to
> plant the seed ahead of time so that we could start thinking about
> something that's a little more clean than an env variable.

Of course, I appreciate the heads-up.  Like I said, the key is to make
sure the existing API functions work correctly regardless of the
kernel support, we can sort out test problems after that; if we have
to make the tests more permissive, we can do that.

-- 
paul moore
www.paul-moore.com

-- 
You received this message because you are subscribed to the Google Groups 
"libseccomp" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to libseccomp+unsubscr...@googlegroups.com.
To post to this group, send email to libseccomp@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to