Re: seccomp ptrace selftest failures with 4.4-stable [Was: Re: LTS testing with latest kselftests - some failures]

[Kees Cook]

On Thu, Jun 22, 2017 at 7:40 PM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> On Thu, Jun 22, 2017 at 6:52 PM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> On Thu, Jun 22, 2017 at 10:50:43AM -0700, Kees Cook wrote:
>>> On Thu, Jun 22, 2017 at 10:49 AM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>>> > On Thu, Jun 22, 2017 at 10:09 AM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
>>> >> On 06/22/2017 10:53 AM, Kees Cook wrote:
>>> >>> On Thu, Jun 22, 2017 at 9:18 AM, Sumit Semwal <sumit.semwal@xxxxxxxxxx> wrote:
>>> >>>> Hi Kees, Andy,
>>> >>>>
>>> >>>> On 15 June 2017 at 23:26, Sumit Semwal <sumit.semwal@xxxxxxxxxx> wrote:
>>> >>>>> 3. 'seccomp ptrace hole closure' patches got added in 4.7 [3] -
>>> >>>>> feature and test together.
>>> >>>>> - This one also seems like a security hole being closed, and the
>>> >>>>> 'feature' could be a candidate for stable backports, but Arnd tried
>>> >>>>> that, and it was quite non-trivial. So perhaps  we'll need some help
>>> >>>>> from the subsystem developers here.
>>> >>>>
>>> >>>> Could you please help us sort this out? Our goal is to help Greg with
>>> >>>> testing stable kernels, and currently the seccomp tests fail due to
>>> >>>> missing feature (seccomp ptrace hole closure) getting tested via
>>> >>>> latest kselftest.
>>> >>>>
>>> >>>> If you feel the feature isn't a stable candidate, then could you
>>> >>>> please help make the test degrade gracefully in its absence?
>>> >>>
>>> >>> I don't really want to have that change be a backport -- it's quite
>>> >>> invasive across multiple architectures.
>>> >>>
>>> >>> I would say just add a kernel version check to the test. This is
>>> >>> probably not the only selftest that will need such things. :)
>>> >>
>>> >> Adding release checks to selftests is going to problematic for maintenance.
>>> >> Tests should fail gracefully if feature isn't supported in older kernels.
>>> >>
>>> >> Several tests do that now and please find a way to check for dependencies
>>> >> and feature availability and fail the test gracefully. If there is a test
>>> >> that can't do that for some reason, we can discuss it, but as a general
>>> >> rule, I don't want to see kselftest patches that check release.
>>> >
>>> > If a future kernel inadvertently loses the new feature and degrades to
>>> > the behavior of old kernels, that would be a serious bug and should be
>>> > caught.
>>>
>>> Right. I really think stable kernels should be tested with their own
>>> selftests. If some test is needed in a stable kernel it should be
>>> backported to that stable kernel.
>>
>> Well, ideally all new features added to the kernel should be able to be
>> detected by userspace somehow if they are present or not.
>>
>> How do you expect a program to know if a feature has "failed" or is just
>> "not enabled/present in this kernel"?  Normally with syscalls this is
>> easy, same for sysfs changes.  Is seccomp in the bad state where there
>> is no way to detect the two different states here?  How is userspace
>> supposed to deal with that?
>>
>> We make fun of glibc having a zillion crazy tests to determine kernel
>> features, and recently, just not wrapping new syscalls at all because
>> they are just frustrated at the compatibility issues over time.  Let's
>> not make their life any harder than it has to be please.
>>
>> I don't see how any of the kselftest programs are any different than any
>> other userspace program that wants to use our kernel api, and as such,
>> any version of kselftest should be able to successfully run on any
>> kernel release.  If not, then we messed up in how we either wrote the
>> test, or how we added a new kernel api.  Neither is acceptable.
>
> That's a fair point.  We could add SECCOMP_GET_FEATURES that returns a
> mask of otherwise-difficult-to-probe features.

But that's silly. The self tests includes all kinds of bug fix tests,
and adding each of those to some features list seems like crazy
overkill. And every API in the kernel is going to do this?

> Greg, for context, the issue here is that we made what was arguably a
> design error in seccomp's interaction with ptrace.  After determining
> that fixing it solved a bunch of problems and didn't break any user
> programs, we fixed it.  There might be new code that relies on the fix
> being present in the sense that it would be insecure without the fix.
>
> The problem is that the fix is moderately intrusive and doesn't seem
> like a great candidate for backporting, although we could plausibly do
> it.

Even if we did all this for seccomp, we're left with the general case.
This is not a situation unique to seccomp. Behaviors and bug fix tests
are added to selftests over time, and not all of those things are
backport-worthy. If a certain test is desired for a stable kernel, we
should backport the test.

-Kees

-- 
Kees Cook
Pixel Security