Re: [PATCH 0/9] mm: Hardened usercopy

On Sun, Jul 10, 2016 at 5:03 AM, PaX Team [off-list ref] wrote:

On 10 Jul 2016 at 11:16, Ingo Molnar wrote:

quoted

* PaX Team [off-list ref] wrote:

quoted

On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:

quoted

I like the series, but I have one minor nit to pick.  The effect of this
series is to harden usercopy, but most of the code is really about
infrastructure to validate that a pointed-to object is valid.

actually USERCOPY has never been about validating pointers. its sole purpose is
to validate the *size* argument of copy*user calls, a very specific form of
runtime bounds checking.

What this code has been about originally is largely immaterial, unless you can
formulate it into a technical argument.

we design defense mechanisms for specific and clear purposes, starting with
a threat model, evaluating defense options based on various criteria, etc.
USERCOPY underwent this same process and taking it out of its original context
means that all you get in the end is cargo cult security (wouldn't be the first
time it has happened (ExecShield, ASLR, etc)).

that said, i actually started that discussion but for some reason you chose
not to respond to that one part of my mail so let me ask it again:

  what kind of checks are you thinking of here? and more fundamentally, against
  what kind of threats?

as far as i'm concerned, a defense mechanism is only as good as its underlying
threat model. by validating pointers (for yet to be stated security related
properties) you're presumably assuming some kind of threat and unless stated
clearly what that threat is (unintended pointer modification through memory
corruption and/or other bugs?) noone can tell whether the proposed defense
mechanism will actually be effective in preventing exploitation. it is the
worst kind of defense that doesn't actually achieve its stated goals, that
way lies false sense of security and i hope noone here is in that business.

I'm imaging security bugs that involve buffer length corruption but
that don't call copy_to/from_user.  Hardened usercopy shuts
expoitation down if the first use of the corrupt size is
copy_to/from_user or similar.  I bet that a bit better coverage could
be achieved by instrumenting more functions.

To be clear: I'm not objecting to calling the overall feature hardened
usercopy or similar.  I object to
CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR.  That feature is *used* for
hardened usercopy but is not, in and of itself, a usercopy thing.
It's an object / memory range validation thing.  So we'll feel silly
down the road if we use it for something else and the config option
name has nothing to do with the feature.

quoted

quoted

[...] like the renaming of .data..read_only to .data..ro_after_init which also
had nothing to do with init but everything to do with objects being conceptually
read-only...

.data..ro_after_init objects get written to during bootup so it's conceptually
quite confusing to name it "read-only" without any clear qualifiers.

That it's named consistently with its role of "read-write before init and read
only after init" on the other hand is not confusing at all. Not sure what your
problem is with the new name.

the new name reflects a complete misunderstanding of the PaX feature it was based
on (typical case of cargo cult security). in particular, the __read_only facility
in PaX is part of a defense mechanism that attempts to solve a specific problem
(like everything else) and that problem has nothing whatsoever to do with what
happens before/after the kernel init process. enforcing read-ony kernel memory at
the end of kernel initialization is an implementation detail only and wasn't even
true always (and still isn't true for kernel modules for example): in the linux 2.4
days PaX actually enforced read-only kernel memory properties in startup_32 already
but i relaxed that for the 2.6+ port as the maintenance cost (finding out and
handling new exceptional cases) wasn't worth it.

also naming things after their implementation is poor taste and can result in
even bigger problems down the line since as soon as the implementation changes,
you will have a flag day or have to keep a bad name. this is a lesson that the
REFCOUNT submission will learn too since the kernel's atomic*_t types (an
implementation detail) are used extensively for different purposes, instead of
using specialized types (kref is a good example of that). for .data..ro_after_init
the lesson will happen when you try to add back the remaining pieces from PaX,
such as module handling and not-always-const-in-the-C-sense objects and associated
accessors.

The name is related to how the thing works.  If I understand
correctly, in PaX, the idea is to make some things readonly and use
pax_open_kernel(), etc to write it as needed.  This is a nifty
mechanism, but it's *not* what .data..ro_after_init does upstream.  If
I mark something __ro_after_init, then I can write it freely during
boot, but I can't write it thereafter.  In contrast, if I put
something in .rodata (using 'const', for example), then I must not
write it *at all* unless I use special helpers (kmap, pax_open_kernel,
etc).  So the practical effect from a programer's perspective of
__ro_after_init is quite different from .rodata, and I think the names
should reflect that.

(And yes, the upstream kernel should soon have __ro_after_init working
in modules.  And the not-always-const-in-the-C-sense objects using
accessors will need changes to add those accessors, and we can and
should change the annotation on the object itself at the same time.
But if I mark something __ro_after_init, I can write it using normal C
during init, and there's nothing wrong with that.)

--Andy