[GIT PULL] usercopy whitelisting for v4.15-rc1

[GIT PULL] usercopy whitelisting for v4.15-rc1

[Kees Cook]

Hi Linus,

Please pull these hardened usercopy changes for v4.15-rc1. This
significantly narrows the areas of memory that can be copied to/from
userspace in the face of usercopy bugs by adding explicit whitelisting for
slab cache regions. This has lived in -next for quite some time without
major problems, but there were some late-discovered missing whitelists,
so a fallback mode was added just to make sure we don't break anything. I
expect to remove the fallback mode in a release or two.

(Sorry if this pull request is a duplicate: I just don't want to miss
the merge window, given its potential for being shorter than usual.)

Thanks!

-Kees

The following changes since commit 9e66317d3c92ddaab330c125dfe9d06eee268aff:

  Linux 4.14-rc3 (2017-10-01 14:54:54 -0700)

are available in the git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/usercopy-v4.15-rc1

for you to fetch changes up to 3889a28c449c01cebe166e413a58742002c2352b:

  lkdtm: Update usercopy tests for whitelisting (2017-11-08 15:40:04 -0800)

----------------------------------------------------------------
Currently, hardened usercopy performs dynamic bounds checking on slab
cache objects. This is good, but still leaves a lot of kernel memory
available to be copied to/from userspace in the face of bugs. To further
restrict what memory is available for copying, this creates a way to
whitelist specific areas of a given slab cache object for copying to/from
userspace, allowing much finer granularity of access control. Slab caches
that are never exposed to userspace can declare no whitelist for their
objects, thereby keeping them unavailable to userspace via dynamic copy
operations. (Note, an implicit form of whitelisting is the use of constant
sizes in usercopy operations and get_user()/put_user(); these bypass
hardened usercopy checks since these sizes cannot change at runtime.)

----------------------------------------------------------------
David Windsor (23):
      usercopy: Prepare for usercopy whitelisting
      usercopy: Enforce slab cache usercopy region boundaries
      usercopy: Mark kmalloc caches as usercopy caches
      dcache: Define usercopy region in dentry_cache slab cache
      vfs: Define usercopy region in names_cache slab caches
      vfs: Copy struct mount.mnt_id to userspace using put_user()
      ext4: Define usercopy region in ext4_inode_cache slab cache
      ext2: Define usercopy region in ext2_inode_cache slab cache
      jfs: Define usercopy region in jfs_ip slab cache
      befs: Define usercopy region in befs_inode_cache slab cache
      exofs: Define usercopy region in exofs_inode_cache slab cache
      orangefs: Define usercopy region in orangefs_inode_cache slab cache
      ufs: Define usercopy region in ufs_inode_cache slab cache
      vxfs: Define usercopy region in vxfs_inode slab cache
      cifs: Define usercopy region in cifs_request slab cache
      scsi: Define usercopy region in scsi_sense_cache slab cache
      net: Define usercopy region in struct proto slab cache
      ip: Define usercopy region in IP proto slab cache
      caif: Define usercopy region in caif proto slab cache
      sctp: Define usercopy region in SCTP proto slab cache
      sctp: Copy struct sctp_sock.autoclose to userspace using put_user()
      fork: Define usercopy region in mm_struct slab caches
      fork: Define usercopy region in thread_stack slab caches

Kees Cook (8):
      net: Restrict unwhitelisted proto caches to size 0
      fork: Provide usercopy whitelisting for task_struct
      x86: Implement thread_struct whitelist for hardened usercopy
      arm64: Implement thread_struct whitelist for hardened usercopy
      arm: Implement thread_struct whitelist for hardened usercopy
      usercopy: Allow for temporary fallback for non-whitelisted usercopy
      usercopy: Restrict non-usercopy caches to size 0
      lkdtm: Update usercopy tests for whitelisting

Paolo Bonzini (2):
      kvm: whitelist struct kvm_vcpu_arch
      kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

 arch/Kconfig                       | 11 +++++
 arch/arm/Kconfig                   |  1 +
 arch/arm/include/asm/processor.h   |  7 +++
 arch/arm64/Kconfig                 |  1 +
 arch/arm64/include/asm/processor.h |  8 ++++
 arch/x86/Kconfig                   |  1 +
 arch/x86/include/asm/processor.h   |  8 ++++
 arch/x86/kvm/x86.c                 |  7 +--
 drivers/misc/lkdtm.h               |  4 +-
 drivers/misc/lkdtm_core.c          |  4 +-
 drivers/misc/lkdtm_usercopy.c      | 88 +++++++++++++++++++++-----------------
 drivers/scsi/scsi_lib.c            |  9 ++--
 fs/befs/linuxvfs.c                 | 14 +++---
 fs/cifs/cifsfs.c                   | 10 +++--
 fs/dcache.c                        |  9 ++--
 fs/exofs/super.c                   |  7 ++-
 fs/ext2/super.c                    | 12 +++---
 fs/ext4/super.c                    | 12 +++---
 fs/fhandle.c                       |  3 +-
 fs/freevxfs/vxfs_super.c           |  8 +++-
 fs/jfs/super.c                     |  8 ++--
 fs/orangefs/super.c                | 15 ++++---
 fs/ufs/super.c                     | 13 +++---
 include/linux/sched/task.h         | 14 ++++++
 include/linux/slab.h               | 27 +++++++++---
 include/linux/slab_def.h           |  3 ++
 include/linux/slub_def.h           |  3 ++
 include/linux/stddef.h             |  2 +
 include/net/sctp/structs.h         |  9 +++-
 include/net/sock.h                 |  2 +
 kernel/fork.c                      | 31 +++++++++++---
 mm/slab.c                          | 35 ++++++++++++---
 mm/slab.h                          |  8 +++-
 mm/slab_common.c                   | 54 ++++++++++++++++++-----
 mm/slub.c                          | 46 ++++++++++++++++----
 mm/usercopy.c                      | 12 ++++++
 net/caif/caif_socket.c             |  2 +
 net/core/sock.c                    |  4 +-
 net/ipv4/raw.c                     |  2 +
 net/ipv6/raw.c                     |  2 +
 net/sctp/socket.c                  | 10 ++++-
 security/Kconfig                   | 12 ++++++
 virt/kvm/kvm_main.c                |  7 ++-
 43 files changed, 407 insertions(+), 138 deletions(-)

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Linus Torvalds]

On Fri, Nov 17, 2017 at 8:54 AM, Kees Cook <keescook@chromium.org> wrote:
>
> (Sorry if this pull request is a duplicate: I just don't want to miss
> the merge window, given its potential for being shorter than usual.)

Honestly, these things always end up waiting to the end for me, simply
because they are scary, and I don't trust them, so I feel I need to
spend time on them.

And when I pull 20+ other pull requests a day, I don't have _time_ to
spend time on them.

They are scary because:

 - they touch core stuff

 - I don't trust security people to do sane things

 - they tend to come in as a "fait accompli" with a shit-ton of random
arbitrary rules, and are still likely to not be complete.

which just makes these pull requests very painful.

We had a ton of issues with the original hardened usercopy just doing
bad things.

We _still_ have outstanding issues with the structure randomization
corrupting the kernel.

These "hardening" things really seem to be a source of random bugs,
and they haven't been extensively tested, and the people involved
quite often don't seem to care about basic cleanliness (because
"security is so important that nothing else matters").

Honestly, I'm unlikely to pull this at all this merge window, simply
because I won't have time for it. This merge window is not going to be
one where I can take a leisurely look at something like this.

If you can make a smaller pull request that introduces the
infrastructure, but that _obviously_ cannot actually break anything,
that would be more likely to be palatable.

Because right now I'm in "the last hardening feature has an unknown
breakage that nobody knows how to even get to the bottom of, I'm _so_
not interested in another of these things" mode.

                 Linus

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Paolo Bonzini]

On 17/11/2017 18:35, Linus Torvalds wrote:
> Honestly, I'm unlikely to pull this at all this merge window, simply
> because I won't have time for it. This merge window is not going to be
> one where I can take a leisurely look at something like this.
> 
> If you can make a smaller pull request that introduces the
> infrastructure, but that _obviously_ cannot actually break anything,
> that would be more likely to be palatable.

As someone that was actually bitten by this stuff, and had a closer look
at the usercopy whitelisting stuff...  This one is really fail-fast
(oopses all around if you forget to patch something), and with hardly
any configuration dependency.  It's certainly a lot less scary to me
than the GCC plugin stuff.

But I don't want to ruin your Thanksgiving, so if Kees and/or you choose
not to do this pull request---please do pull a subset, even after -rc1.
It's easy enough to drop the final patch that changes whitelisting to
blacklisting, and it'd be one less series bouncing around and touching
files in several subsystems.

My 2 cents,

Paolo

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Kees Cook]

On Fri, Nov 17, 2017 at 9:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 17/11/2017 18:35, Linus Torvalds wrote:
>> Honestly, I'm unlikely to pull this at all this merge window, simply
>> because I won't have time for it. This merge window is not going to be
>> one where I can take a leisurely look at something like this.
>>
>> If you can make a smaller pull request that introduces the
>> infrastructure, but that _obviously_ cannot actually break anything,
>> that would be more likely to be palatable.
>
> As someone that was actually bitten by this stuff, and had a closer look
> at the usercopy whitelisting stuff...  This one is really fail-fast
> (oopses all around if you forget to patch something), and with hardly

This is why I introduced the fallback mode: with both kvm and sctp
(ipv6) not noticed until late in the development cycle, I became much
less satisfied it had gotten sufficient testing. I wanted to make sure
there was a way for the series to land without actually breaking
things due to any missed whitelists.

> any configuration dependency.  It's certainly a lot less scary to me
> than the GCC plugin stuff.

Agreed: this is a different type of change entirely. The GCC plugins
tend to be pretty invasive and non-discoverable. I prefer stuff like
this series, which is all visible in the code.

> But I don't want to ruin your Thanksgiving, so if Kees and/or you choose
> not to do this pull request---please do pull a subset, even after -rc1.
> It's easy enough to drop the final patch that changes whitelisting to
> blacklisting, and it'd be one less series bouncing around and touching
> files in several subsystems.

With the fallback mode, missed whitelists generate a WARN and are
allowed, so this series effectively only introduces tight controls on
the places where a whitelist is specifically introduced. And I went to
great lengths to document each whitelist usage in the commit logs.

I would agree it would be nice to get at least a subset of this in,
though. Linus, what would make you most comfortable?

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Linus Torvalds]

On Fri, Nov 17, 2017 at 12:35 PM, Kees Cook <keescook@chromium.org> wrote:
>
> This is why I introduced the fallback mode: with both kvm and sctp
> (ipv6) not noticed until late in the development cycle, I became much
> less satisfied it had gotten sufficient testing.

So honestly, this is the kind of completely unacceptable "security
person" behavior that we had with the original user access hardening
too, and made that much more painful than it ever should have been.

IT IS NOT ACCEPTABLE when security people set magical new rules, and
then make the kernel panic when those new rules are violated.

That is pure and utter bullshit. We've had more than a quarter century
_without_ those rules, you don't then suddenly walz in and say "oh,
everbody must do this, and if you haven't, we will kill the kernel".

The fact that you "introduced the fallback mode" late in that series
just shows HOW INCREDIBLY BROKEN the series started out.

Seriously.

As a security person, you need to repeat this mantra:

  "security problems are just bugs"

and you need to _internalize_ it, instead of scoff at it.

The important part about "just bugs" is that you need to understand
that the patches you then introduce for things like hardening are
primarly for DEBUGGING.

I'm not at all interested in killing processes. The only process I'm
interested in is the _development_ process, where we find bugs and fix
them.

As long as you see your hardening efforts primarily as a "let me kill
the machine/process on bad behavior", I will stop taking those shit
patches.

I'm deadly serious about this.

Some security people have scoffed at me when I say that security
problems are primarily "just bugs".

Those security people are f*cking morons.

Because honestly, the kind of security person who doesn't accept that
security problems are primarily just bugs, I don't want to work with.
If you don't see your job as "debugging first", I'm simply not
interested.

So I think the hardening project needs to really take a good look at
itself in the mirror.

Because the primary focus should be "debugging". The primary focus
should be "let's make sure the kernel released in a year is better
than the one released today".

And the primary focus right now seems to be "let's kill things for
bugs". That's wrong.

And I'm _so_ not interested in that. It makes me go "no, I will not
pull that shit, it's not safe for me, and it's not safe for our
users".

So the hardening efforts should instead _start_ from the standpoint of
"let's warn about what looks dangerous, and maybe in a _year_ when
we've warned for a long time, and we are confident that we've actually
caught all the normal cases, _then_ we can start taking more drastic
measures".

See the difference?

Stop this idiotic "kill on sight, ask questions later".

Because it's wrong.

> I would agree it would be nice to get at least a subset of this in,
> though. Linus, what would make you most comfortable?

Right now, the biggest problem for me is that the whole thing makes me
uncomfortable, because I think the people involved are coming from a
completely unacceptable model to begin with.

And we had this exact issue with the _previous_ user mode access
hardening. People apparently didn't learn a goddamn thing.

                     Linus

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Kees Cook]

On Fri, Nov 17, 2017 at 1:13 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> As long as you see your hardening efforts primarily as a "let me kill
> the machine/process on bad behavior", I will stop taking those shit
> patches.

Yes, this is entirely clear. This is why I adjusted this series (in
multiple places) to use WARN, etc etc. And why I went to great lengths
to document the rationale, effects, and alloc/use paths so when
something went wrong it would be easy to see what was happening and
why.

> So the hardening efforts should instead _start_ from the standpoint of
> "let's warn about what looks dangerous, and maybe in a _year_ when
> we've warned for a long time, and we are confident that we've actually
> caught all the normal cases, _then_ we can start taking more drastic
> measures".

Understood: I think my main flaw in helping bring these defenses to
the kernel has been thinking they can be fully tested during a single
development cycle, and this mistake was made quite clear this cycle,
which is why I adjusted the series like I did.

> Right now, the biggest problem for me is that the whole thing makes me
> uncomfortable, because I think the people involved are coming from a
> completely unacceptable model to begin with.
>
> And we had this exact issue with the _previous_ user mode access
> hardening. People apparently didn't learn a goddamn thing.

Well, I'd like to think I did learn something, since I fixed up this
series _before_ you yelled at me. :)

I'll make further adjustments and try again for v4.16.

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Matthew Garrett]

On Fri, Nov 17, 2017 at 01:13:10PM -0800, Linus Torvalds wrote:
> So the hardening efforts should instead _start_ from the standpoint of
> "let's warn about what looks dangerous, and maybe in a _year_ when
> we've warned for a long time, and we are confident that we've actually
> caught all the normal cases, _then_ we can start taking more drastic
> measures".

Can you clarify a little with regard to how you'd have liked this 
patchset to look? With 
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=usercopy-v4.15-rc1&id=93edfb33387772a9ae7693ddf9811280ffc4025b 
it seems pretty like we're at the end goal you've described (default to 
a mode that generates a warning rather than returning an error), so is 
it just that this appeared at the end of the patchset development 
process rather than being there from the beginning?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Matthew Garrett]

On Mon, Nov 20, 2017 at 12:47:10PM -1000, Linus Torvalds wrote:
> Sorry, on mobile right now, thus nasty HTML email..
> 
> On Nov 20, 2017 09:50, "Matthew Garrett" <mjg59@srcf.ucam.org> wrote:
> 
> 
>> Can you clarify a little with regard to how you'd have liked this
>> patchset to look?
> 
> 
> So I think the actual status of the patches is fairly good with the default
> warning.
> 
> But what I'd really like to see is to not have to worry so much about these
> hardening things. The last set of user access hardening really was more
> painful than it might have been.

Sure, and Kees learned from that experience and added the default 
fallback in response to it. Let's reward people for learning from past 
problems rather than screaming at them :)

>From a practical perspective this does feel like a completely reasonable 
request - when changing the semantics of kernel APIs in ways that aren't 
amenable to automated analysis, doing so in a way that generates 
warnings rather than triggering breakage is pretty clearly a preferable 
approach. But these features often start off seeming simple and then 
devolving into rounds of "ok just one more fix and we'll have 
everything" and by then it's easy to have lost track of the amount of 
complexity that's developed as a result. Formalising the Right Way of 
approaching these problems would possibly help avoid this kind of 
problem in future - I'll try to write something up for 
Documentation/process.

> And largely due to that I was really dreading pulling this one - and then
> with 20+ pulls a day because I really wanted to get everything big merged
> before travel, I basically ran out of time.
> 
> Part of that is probably also because the 4.15 merge window actually ended
> up bigger than I expected. I was perhaps naive, but I expected that because
> of 4.14 being LTS, this release would be smaller (like 4.9 vs 4.10) but
> that never happened.
> 
> So where I'd really like to be is simply that these pulls wouldn't be so
> nerve wracking for me. And that's largely me worrying about the approach
> people are taking, which is why I then reacted so strongly to the whole
> "warnings came later".
> 
> Sorry for the strong words.

This one seems unfortunate in that a lot of people interpreted it as 
"Kees submits bad code", and I think that does have an impact on 
people's enthusiasm for submitting more complex or controversial work. 
The number of people willing to work on security stuff is limited enough 
for various reasons, let's try to keep hold of the ones we have!

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Kees Cook]

On Mon, Nov 20, 2017 at 3:29 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> From a practical perspective this does feel like a completely reasonable
> request - when changing the semantics of kernel APIs in ways that aren't
> amenable to automated analysis, doing so in a way that generates
> warnings rather than triggering breakage is pretty clearly a preferable
> approach. But these features often start off seeming simple and then
> devolving into rounds of "ok just one more fix and we'll have
> everything" and by then it's easy to have lost track of the amount of
> complexity that's developed as a result. Formalising the Right Way of
> approaching these problems would possibly help avoid this kind of
> problem in future - I'll try to write something up for
> Documentation/process.

I'm always trying to balance the requests from both ends of the
security defense spectrum. One of the most common requests I get from
people who are strongly interested in the defenses is "can this please
be enabled by default?" And then I have to explain that it sometimes
takes time for code to shake out, and it sometimes takes time for
developers to trust it, etc. This is rarely a comfort to them, but
they tend to be glad they can turn some config knob to enable the
"strongest" version, etc, because for them, a false positive is no big
deal. At the other end is the requirement that new stuff should not
break the system. Both are reasonable perspectives, but if we violate
the latter, the defense will never end up in the kernel in the first
place.

The implicit process tends to be: land a defense that is optional,
distros enable it by default, kernel enables it by default. My sense
is that the span for these steps is about 3 years. There are countless
examples, but one of the more recent is x86 KASLR. Optional (Jun 2014,
v3.14), then distro default (e.g. Ubuntu 16.10, Oct 2016), then kernel
default (Jul 2017, v4.12). (And, in my opinion, this takes way too
long. But I'd rather have the defense at all.)

The trouble tends to be even in the "optional" phase, where it may be
optional but still very disruptive. If you were using KASLR but there
was a bug, it was basically going to be fatal to the system. So, my
historical perspective for other less disruptive defenses was "oh
good, it's only BUG(), that's WAY better than taking out the entire
system", but as Linus has pointed out in several threads, BUG() can
frequently lead to that too, which is not acceptable.

A variation on "new stuff should not break the system" comes from
things that LOOK like they can't break the system (i.e. disallowing
some pathological condition that would seem to be entirely reasonable
to disallow), and then discovering that stuff does, actually, use a
pathological condition in the real world (e.g. refcount underflows,
memcpy beyond the end of a short source string, 3rd party drivers
reading directly from userspace memory). And then there's just plain
bugs in defenses (e.g. missed usercopy whitelist for SCTPv6). Handling
these conditions means that in addition to being optional, a new
defense frequently has to also be WARN()-only initially.

Though, even then, there is a spectrum of defense capabilities. KASLR
couldn't WARN() initially since it's an architectural state. The
refcount_t API doesn't really need to be stronger than WARN() because
it can defend against overflow while leaving everything running fine.
And here, as already done, usercopy whitelisting needs to WARN() for
its initial implementation to shake out any hard-to-find cases before
becoming more aggressive.

With all this in mind, it can be tricky to find a way to provide the
right level of initial defense behavior that also has a knob for the
users that want to accept the risk of false positives. I'll keep
trying to get it right and keeping helping others find the right path.
We'll get there, steady progress, etc, etc. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Jason A. Donenfeld]

Hi Linus,

On Fri, Nov 17, 2017 at 10:13 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> As a security person, you need to repeat this mantra:
>
>   "security problems are just bugs"
>
> and you need to _internalize_ it, instead of scoff at it.

It might be news to you that actually some security people scoff at
other security peoples' obsession with "security bugs".

Security people like to rate the "utility" of a bug in a security
setting, usually from from "does nothing" (benign) to "allows code
execution" (dangerous/useful), with something like "crashes system"
somewhere in between those two poles. (You may not agree with this
scale, but indeed security people tend to think that a crash is safer
than allowing malicious code execution.)

The security industry is largely obsessed by finding (and
selling/using/patching/reporting/showcasing/stockpiling/detecting/stealing)
these "dangerous/useful" variety of bugs. And this obsession is
continually fulfilled because bugs keep happening -- which is just the
nature of software development -- and so this "security bug"
infatuation continues. It even leads to people upset with you that you
don't care about CVEs and so forth, because they're so fixated on
individual bugs and their security impact.

So, as I mentioned, it may come as a surprise to you that some
security people don't really care for that mentality. As Bas Alberts
wrote [1], "Bugs are irrelevant. Yet our industry is fatally focused
on what is essentially vulnerability masturbation." So what's the
alternative to obsessing over each individual software bug?

In the context of the kernel, the solution from Spender and Pipacs,
and more recently "adopted" by Kees and his KSPP project, has been to
try to eliminate the "security utility" of bugs. In the best case,
this means making bugs that were formerly "dangerous/useful" be
"benign". In the second best case, it usually means making bugs that
were formerly "dangerous/useful" be "irreliable" or "crashes system".
The idea is to completely eliminate the security impact of entire
*classes* of bugs. It acknowledges that programmers will continue to
make programming bugs, as they do, and seeks to make the kernel safer,
from a security perspective, in spite of the continued existence of
these bugs.

> Because the primary focus should be "debugging". The primary focus
> should be "let's make sure the kernel released in a year is better
> than the one released today".

In light of the above, this then is where there might be an
unfortunate disconnect. Sure, many hardening features that turn
dangerous bugs into less dangerous bugs have the quality of unearthing
individual bugs. But many other aspects are rightfully focused on
mitigating the security impact future unforeseen, undetected bugs.
>From a security perspective, please try to see beyond the individual
bug obsession, and consider the larger picture. After all, if the
security impact of all kernel bugs is reduced to nil, your mantra that
all bugs are just bugs will be even more true.

Jason

[1] https://lists.immunityinc.com/pipermail/dailydave/2015-August/000976.html

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Lukas Wunner]

On Mon, Nov 20, 2017 at 04:42:46PM -0800, Kees Cook wrote:
> I'm always trying to balance the requests from both ends of the
> security defense spectrum. One of the most common requests I get from
> people who are strongly interested in the defenses is "can this please
> be enabled by default?" And then I have to explain that it sometimes
> takes time for code to shake out, and it sometimes takes time for
> developers to trust it, etc. This is rarely a comfort to them, but
> they tend to be glad they can turn some config knob to enable the
> "strongest" version, etc, because for them, a false positive is no big
> deal. At the other end is the requirement that new stuff should not
> break the system. Both are reasonable perspectives, but if we violate
> the latter, the defense will never end up in the kernel in the first
> place.

I think Linus' objections must be seen in a broader context,
the last months we've seen a large influx of semi-automatically
generated hardening patches (constification mostly);  The volume
of these was at times overwhelming, in some cases they were dropped
on the mailing lists in a fire-and-forget fashion and there are
multiple documented cases where these patches broke things and the
community was left to repair the fallout.

When LWN published the 4.14 patch statistics (which some contributors
seem to mistake for a high score), Alexandre Belloni (+cc) rightfully
raised a complaint and included links to some of the broken patches:

https://lwn.net/Articles/736578/#CommAnchor737081

There is a growing sense that hardening patches (which are generally
desirable of course) are forced into the kernel without due diligence.
Objections against tone notwithstanding, making that concern known in
a form with greater visibility than an LWN comment was appropriate by
Linus.

It is most unfortunate that these concerns happened to be adressed to
you personally and stalled your patches (the quality of which I cannot
judge), even though they must (in my opinion) be interpreted in the
context of this larger issue.

It is likewise most unfortunate that this allowed bloggers, journalists
and HN know-it-alls with zero commits in the kernel and zero participation
on the mailing lists to focus on tone and whatnot, but completely
ignore what this is really about.

Thanks,

Lukas

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Linus Torvalds]

[ this is just a resend of yesterday's mobile html version that got
rejected by the lists, sorry for the duplication ]

On Mon, Nov 20, 2017 at 9:50 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> Can you clarify a little with regard to how you'd have liked this
> patchset to look?

So I think the actual status of the patches is fairly good with the
default warning.

But what I'd really like to see is to not have to worry so much about
these hardening things. The last set of user access hardening really
was more painful than it might have been.

And largely due to that I was really dreading pulling this one - and
then with 20+ pulls a day because I really wanted to get everything
big merged before travel, I basically ran out of time.

Part of that is probably also because the 4.15 merge window actually
ended up bigger than I expected. I was perhaps naive, but I expected
that because of 4.14 being LTS, this release would be smaller (like
4.9 vs 4.10) but that never happened.

So where I'd really like to be is simply that these pulls wouldn't be
so nerve wracking for me. And that's largely me worrying about the
approach people are taking, which is why I then reacted so strongly to
the whole "warnings came later".

Sorry for the strong words.

     Linus

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Linus Torvalds]

[ This turned longer than it should have. I guess jet lag is a good thing ]

On Tue, Nov 21, 2017 at 3:48 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> It might be news to you that actually some security people scoff at
> other security peoples' obsession with "security bugs".

Heh. I'm not actually surprised. It's just that the public "look at
this security bug" ones are the ones  you see.

> The security industry is largely obsessed by finding (and
> selling/using/patching/reporting/showcasing/stockpiling/detecting/stealing)
> these "dangerous/useful" variety of bugs. And this obsession is
> continually fulfilled because bugs keep happening -- which is just the
> nature of software development -- and so this "security bug"
> infatuation continues. It even leads to people upset with you that you
> don't care about CVEs and so forth, because they're so fixated on
> individual bugs and their security impact.

Agreed.

> So what's the
> alternative to obsessing over each individual software bug?
>
> In the context of the kernel, the solution from Spender and Pipacs,
> and more recently "adopted" by Kees and his KSPP project, has been to
> try to eliminate the "security utility" of bugs.

And the thing is, I obviously agree very much about the whole "let's
have multiple layers of security even within the kernel, so that
random individual bugs don't end up being so exploitable". Bugs will
happen, let's aim to limit their damage.

To turn them "benign" in your words.

So I should be thrilled pink about the hardening efforts, right?

Well, I would - except for what "benign" means in that context, and
how security people have very different expectations from users - and
how those are both different from developers.

>From a security standpoint, when you find an invalid access, and you
mitigate it, you've done a great job, and your hardening was
successful and you're done. "Look ma, it's not a security issue any
more", and you can basically ignore it as "just another bug" that is
now in a class that is no longer your problem.

So to you, the big win is when the access is _stopped_. That's the end
of the story from a security standpoint - at least if you are one of
those bad security people who don't care about anything else.

But from a developer standpoint, things _really_ are not done. Not
even close. From a developer standpoint, the bad access was just a
symptom, and it needs to be reported, and debugged, and fixed, so that
the bug actually gets corrected.

So from a developer standpoint, the end point of  hardening is just
the starting point, and when _you_ think you're done, we're really
only getting started.

And from a _user_ standpoint, it's something else altogether. For a
user, pretty much EVERY SINGLE TIME, it wasn't actually a security
attack at all, it was just a latent bug that got exposed. And the
keyword here is that it was _latent_, and things used to work, and the
hardening patch did something - probably fairly drastic - to turn it
from "dangerous" to "benign" from a security perspective.

So from a user standpoint, the hardening was just a big nasty
annoyance, and probably made their workflow _break_, without actually
helping their case at all, because they never really saw the original
bug as a problem to begin with.

Notice? BIG disconnect in what "hardening" means for three groups, and
in particular, the number one rule of kernel development is that "we
don't break users".

Because without users, your program is pointless, and all the
development work you've done over decades is pointless.

.. and security is pointless too, in the end.

Now, the thing that annoys me and that makes me so _angry_ about this,
is that it shouldn't need to be that huge a disconnect.

It shouldn't need to be a big issue, because pretty much all the work
done for hardening should be able to actually make both the developers
and the users _happier_, instead of just making their lives miserable.

But that does mean that he hardening people need to really see past
that "endpoint" that they were looking at.

For a developer, the hardening effort _could_ be a great boon, in that
it could show nasty bugs early, it could make them easier to report,
and it could add a lot of useful information to that report that makes
them easier to fix too.

And from a user perspective, the hardening work shouldn't have to mean
"the latent bug that I didn't care about now screwed me over and is an
overt bug for me". It might not help the user directly, but if a year
from now, the latent bug that made their machine occasionally go all
wonky is fixed, the hardening effort did end up helping them too.

But what do we need for this to actually happen?

As a developer, I do want the report. But if you killed the user
program in the process, I'm actually _less_ likely to get the report,
because the latent access was most likely in some really rare and
nasty case, or we would have found it already. In the kernel, there's
a high likelihood that it was in a driver, for example. Maybe an
unusual ioctl() that is not getting a huge amount of attention,
because it's one driver ramong thousands, and it's probably not used
every time anyway. But because it's the kernel, and because it's a
driver, it's quite likely that killing the offender will do bad things
to various random locks that were held, or maybe it happens in an
interrupt and the whole machine is now dead if we're unlucky because
there really were some very core locks being held.

And as a user, my unhappiness is obvious. You don't even have to kill
the machine and make it hard to report to make a user unhappy, just
"the new kernel didn't work for me" will make that user skittish about
upgrading the kernel at all.

And the fix really looks fairly straightforward:

 - when adding hardening features, you as a security person should
always see that hardening to be the _endpoint_, but not the immediate
goal.

 - when adding hardening features, the first step should *ALWAYS* be
"just report it". Not killing things, not even stopping the access.
Report it. Nothing else.

"Do no harm" should be your mantra for any new hardening work.

And that "do no harm" may feel antithetical to the whole point. You go
"but t hat doesn't work - then the bug still exists".

But remember - keep your eye on the endpoint, and that this is just
the first step. You need to not piss off users, and you need to not
piss of developers.

Because if you as a security person just piss off users, and piss off
developers, I'm not going to take your work, and I'm going to call you
a bad security person.

Because in the end, those users really do matter. Without those users,
your system may be "secure", but all your security work was still just
masturbation. You didn't do anything useful at all in the end.

So if hardening people can learn to "always report first", then I
think we can all work together.

But that really does mean that you don't start killing processes until
after you've shown that "look, the code to report these things has
been there for months, can we start doing more drastic things now?"

And remember: it's not that the code is  months old. It's that the
code has been RUN BY USERS for months. If it's been in your tree, or
in grsecurity for five years, that doesn't mean a thing. It only means
that hardly anybody actually ever ran it.

If it's been on a random cellphone for a few months, and real users
used it, and had facebook and candy crush running on it, that's a
pretty different deal.

If it's been in a released kernel for a year, and Ubuntu and Fedora
and SuSE had it enabled, and there aren't reports, that's a big deal.

In contrast, if it's been on your server farm for three months, that
means _nothing_. You have pretty much zero coverage of the driver
situation, or of the random apps that people run.

See?

All I need is that the whole "let's kill processes" mentality goes
away, and that people acknowledge that the first step is always "just
report".

Do no harm.

Please.

                Linus

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Randy Dunlap]

On 11/21/2017 05:53 AM, Lukas Wunner wrote:

> There is a growing sense that hardening patches (which are generally
> desirable of course) are forced into the kernel without due diligence.
> Objections against tone notwithstanding, making that concern known in
> a form with greater visibility than an LWN comment was appropriate by
> Linus.

More (qualified) reviewers should also help in patch acceptance.
I expect that's a problem, both in qualifications and time to review.

-- 
~Randy

Re: [GIT PULL] usercopy whitelisting for v4.15-rc1

[Kees Cook]

On Sun, Nov 12, 2017 at 11:29 PM, Kees Cook <keescook@chromium.org> wrote:
> Please pull these hardened usercopy whitelisting changes for v4.15-rc1.
> This significantly narrows the areas of memory that can be copied to/from
> userspace in the face of usercopy bugs.

Just wanted to make sure this pull request was still on your radar.
Let me know if you want me to do a full resend.

Thanks!

-Kees

> The following changes since commit 9e66317d3c92ddaab330c125dfe9d06eee268aff:
>
>   Linux 4.14-rc3 (2017-10-01 14:54:54 -0700)
>
> are available in the git repository at:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/usercopy-v4.15-rc1
>
> for you to fetch changes up to 3889a28c449c01cebe166e413a58742002c2352b:
>
>   lkdtm: Update usercopy tests for whitelisting (2017-11-08 15:40:04 -0800)
>
> ----------------------------------------------------------------
> Currently, hardened usercopy performs dynamic bounds checking on slab
> cache objects. This is good, but still leaves a lot of kernel memory
> available to be copied to/from userspace in the face of bugs. To further
> restrict what memory is available for copying, this creates a way to
> whitelist specific areas of a given slab cache object for copying to/from
> userspace, allowing much finer granularity of access control. Slab caches
> that are never exposed to userspace can declare no whitelist for their
> objects, thereby keeping them unavailable to userspace via dynamic copy
> operations. (Note, an implicit form of whitelisting is the use of constant
> sizes in usercopy operations and get_user()/put_user(); these bypass
> hardened usercopy checks since these sizes cannot change at runtime.)
>
> ----------------------------------------------------------------
> David Windsor (23):
>       usercopy: Prepare for usercopy whitelisting
>       usercopy: Enforce slab cache usercopy region boundaries
>       usercopy: Mark kmalloc caches as usercopy caches
>       dcache: Define usercopy region in dentry_cache slab cache
>       vfs: Define usercopy region in names_cache slab caches
>       vfs: Copy struct mount.mnt_id to userspace using put_user()
>       ext4: Define usercopy region in ext4_inode_cache slab cache
>       ext2: Define usercopy region in ext2_inode_cache slab cache
>       jfs: Define usercopy region in jfs_ip slab cache
>       befs: Define usercopy region in befs_inode_cache slab cache
>       exofs: Define usercopy region in exofs_inode_cache slab cache
>       orangefs: Define usercopy region in orangefs_inode_cache slab cache
>       ufs: Define usercopy region in ufs_inode_cache slab cache
>       vxfs: Define usercopy region in vxfs_inode slab cache
>       cifs: Define usercopy region in cifs_request slab cache
>       scsi: Define usercopy region in scsi_sense_cache slab cache
>       net: Define usercopy region in struct proto slab cache
>       ip: Define usercopy region in IP proto slab cache
>       caif: Define usercopy region in caif proto slab cache
>       sctp: Define usercopy region in SCTP proto slab cache
>       sctp: Copy struct sctp_sock.autoclose to userspace using put_user()
>       fork: Define usercopy region in mm_struct slab caches
>       fork: Define usercopy region in thread_stack slab caches
>
> Kees Cook (8):
>       net: Restrict unwhitelisted proto caches to size 0
>       fork: Provide usercopy whitelisting for task_struct
>       x86: Implement thread_struct whitelist for hardened usercopy
>       arm64: Implement thread_struct whitelist for hardened usercopy
>       arm: Implement thread_struct whitelist for hardened usercopy
>       usercopy: Allow for temporary fallback for non-whitelisted usercopy
>       usercopy: Restrict non-usercopy caches to size 0
>       lkdtm: Update usercopy tests for whitelisting
>
> Paolo Bonzini (2):
>       kvm: whitelist struct kvm_vcpu_arch
>       kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl
>
>  arch/Kconfig                       | 11 +++++
>  arch/arm/Kconfig                   |  1 +
>  arch/arm/include/asm/processor.h   |  7 +++
>  arch/arm64/Kconfig                 |  1 +
>  arch/arm64/include/asm/processor.h |  8 ++++
>  arch/x86/Kconfig                   |  1 +
>  arch/x86/include/asm/processor.h   |  8 ++++
>  arch/x86/kvm/x86.c                 |  7 +--
>  drivers/misc/lkdtm.h               |  4 +-
>  drivers/misc/lkdtm_core.c          |  4 +-
>  drivers/misc/lkdtm_usercopy.c      | 88 +++++++++++++++++++++-----------------
>  drivers/scsi/scsi_lib.c            |  9 ++--
>  fs/befs/linuxvfs.c                 | 14 +++---
>  fs/cifs/cifsfs.c                   | 10 +++--
>  fs/dcache.c                        |  9 ++--
>  fs/exofs/super.c                   |  7 ++-
>  fs/ext2/super.c                    | 12 +++---
>  fs/ext4/super.c                    | 12 +++---
>  fs/fhandle.c                       |  3 +-
>  fs/freevxfs/vxfs_super.c           |  8 +++-
>  fs/jfs/super.c                     |  8 ++--
>  fs/orangefs/super.c                | 15 ++++---
>  fs/ufs/super.c                     | 13 +++---
>  include/linux/sched/task.h         | 14 ++++++
>  include/linux/slab.h               | 27 +++++++++---
>  include/linux/slab_def.h           |  3 ++
>  include/linux/slub_def.h           |  3 ++
>  include/linux/stddef.h             |  2 +
>  include/net/sctp/structs.h         |  9 +++-
>  include/net/sock.h                 |  2 +
>  kernel/fork.c                      | 31 +++++++++++---
>  mm/slab.c                          | 35 ++++++++++++---
>  mm/slab.h                          |  8 +++-
>  mm/slab_common.c                   | 54 ++++++++++++++++++-----
>  mm/slub.c                          | 46 ++++++++++++++++----
>  mm/usercopy.c                      | 12 ++++++
>  net/caif/caif_socket.c             |  2 +
>  net/core/sock.c                    |  4 +-
>  net/ipv4/raw.c                     |  2 +
>  net/ipv6/raw.c                     |  2 +
>  net/sctp/socket.c                  | 10 ++++-
>  security/Kconfig                   | 12 ++++++
>  virt/kvm/kvm_main.c                |  7 ++-
>  43 files changed, 407 insertions(+), 138 deletions(-)
>
> --
> Kees Cook
> Pixel Security



-- 
Kees Cook
Pixel Security

[GIT PULL] usercopy whitelisting for v4.15-rc1

[Kees Cook]

Hi,

Please pull these hardened usercopy whitelisting changes for v4.15-rc1.
This significantly narrows the areas of memory that can be copied to/from
userspace in the face of usercopy bugs.

Thanks!

-Kees

The following changes since commit 9e66317d3c92ddaab330c125dfe9d06eee268aff:

  Linux 4.14-rc3 (2017-10-01 14:54:54 -0700)

are available in the git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/usercopy-v4.15-rc1

for you to fetch changes up to 3889a28c449c01cebe166e413a58742002c2352b:

  lkdtm: Update usercopy tests for whitelisting (2017-11-08 15:40:04 -0800)

----------------------------------------------------------------
Currently, hardened usercopy performs dynamic bounds checking on slab
cache objects. This is good, but still leaves a lot of kernel memory
available to be copied to/from userspace in the face of bugs. To further
restrict what memory is available for copying, this creates a way to
whitelist specific areas of a given slab cache object for copying to/from
userspace, allowing much finer granularity of access control. Slab caches
that are never exposed to userspace can declare no whitelist for their
objects, thereby keeping them unavailable to userspace via dynamic copy
operations. (Note, an implicit form of whitelisting is the use of constant
sizes in usercopy operations and get_user()/put_user(); these bypass
hardened usercopy checks since these sizes cannot change at runtime.)

----------------------------------------------------------------
David Windsor (23):
      usercopy: Prepare for usercopy whitelisting
      usercopy: Enforce slab cache usercopy region boundaries
      usercopy: Mark kmalloc caches as usercopy caches
      dcache: Define usercopy region in dentry_cache slab cache
      vfs: Define usercopy region in names_cache slab caches
      vfs: Copy struct mount.mnt_id to userspace using put_user()
      ext4: Define usercopy region in ext4_inode_cache slab cache
      ext2: Define usercopy region in ext2_inode_cache slab cache
      jfs: Define usercopy region in jfs_ip slab cache
      befs: Define usercopy region in befs_inode_cache slab cache
      exofs: Define usercopy region in exofs_inode_cache slab cache
      orangefs: Define usercopy region in orangefs_inode_cache slab cache
      ufs: Define usercopy region in ufs_inode_cache slab cache
      vxfs: Define usercopy region in vxfs_inode slab cache
      cifs: Define usercopy region in cifs_request slab cache
      scsi: Define usercopy region in scsi_sense_cache slab cache
      net: Define usercopy region in struct proto slab cache
      ip: Define usercopy region in IP proto slab cache
      caif: Define usercopy region in caif proto slab cache
      sctp: Define usercopy region in SCTP proto slab cache
      sctp: Copy struct sctp_sock.autoclose to userspace using put_user()
      fork: Define usercopy region in mm_struct slab caches
      fork: Define usercopy region in thread_stack slab caches

Kees Cook (8):
      net: Restrict unwhitelisted proto caches to size 0
      fork: Provide usercopy whitelisting for task_struct
      x86: Implement thread_struct whitelist for hardened usercopy
      arm64: Implement thread_struct whitelist for hardened usercopy
      arm: Implement thread_struct whitelist for hardened usercopy
      usercopy: Allow for temporary fallback for non-whitelisted usercopy
      usercopy: Restrict non-usercopy caches to size 0
      lkdtm: Update usercopy tests for whitelisting

Paolo Bonzini (2):
      kvm: whitelist struct kvm_vcpu_arch
      kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

 arch/Kconfig                       | 11 +++++
 arch/arm/Kconfig                   |  1 +
 arch/arm/include/asm/processor.h   |  7 +++
 arch/arm64/Kconfig                 |  1 +
 arch/arm64/include/asm/processor.h |  8 ++++
 arch/x86/Kconfig                   |  1 +
 arch/x86/include/asm/processor.h   |  8 ++++
 arch/x86/kvm/x86.c                 |  7 +--
 drivers/misc/lkdtm.h               |  4 +-
 drivers/misc/lkdtm_core.c          |  4 +-
 drivers/misc/lkdtm_usercopy.c      | 88 +++++++++++++++++++++-----------------
 drivers/scsi/scsi_lib.c            |  9 ++--
 fs/befs/linuxvfs.c                 | 14 +++---
 fs/cifs/cifsfs.c                   | 10 +++--
 fs/dcache.c                        |  9 ++--
 fs/exofs/super.c                   |  7 ++-
 fs/ext2/super.c                    | 12 +++---
 fs/ext4/super.c                    | 12 +++---
 fs/fhandle.c                       |  3 +-
 fs/freevxfs/vxfs_super.c           |  8 +++-
 fs/jfs/super.c                     |  8 ++--
 fs/orangefs/super.c                | 15 ++++---
 fs/ufs/super.c                     | 13 +++---
 include/linux/sched/task.h         | 14 ++++++
 include/linux/slab.h               | 27 +++++++++---
 include/linux/slab_def.h           |  3 ++
 include/linux/slub_def.h           |  3 ++
 include/linux/stddef.h             |  2 +
 include/net/sctp/structs.h         |  9 +++-
 include/net/sock.h                 |  2 +
 kernel/fork.c                      | 31 +++++++++++---
 mm/slab.c                          | 35 ++++++++++++---
 mm/slab.h                          |  8 +++-
 mm/slab_common.c                   | 54 ++++++++++++++++++-----
 mm/slub.c                          | 46 ++++++++++++++++----
 mm/usercopy.c                      | 12 ++++++
 net/caif/caif_socket.c             |  2 +
 net/core/sock.c                    |  4 +-
 net/ipv4/raw.c                     |  2 +
 net/ipv6/raw.c                     |  2 +
 net/sctp/socket.c                  | 10 ++++-
 security/Kconfig                   | 12 ++++++
 virt/kvm/kvm_main.c                |  7 ++-
 43 files changed, 407 insertions(+), 138 deletions(-)

-- 
Kees Cook
Pixel Security