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
>>> 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

> 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

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