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

[Kees Cook]

On Sun, Jul 10, 2016 at 8:03 AM, PaX Team <pageexec@xxxxxxxxxxx> wrote:
> i note that this analysis is also missing from this USERCOPY submission except
> for stating what Kees assumed about USERCOPY (and apparently noone could be
> bothered to read the original Kconfig help of it which clearly states that the
> purpose is copy size checking, not some elaborate pointer validation, the latter
> is an implementation detail only and is necessary to be able to derive the
> underlying slab object's intended size).

I read the Kconfig text, but it's not entirely accurate. While size is
being checked, it's all nonsense without also the address, so it's
really an object checker. The original design intent may have been the
slab size checks, but it grew beyond that (both within PaX and within
Grsecurity which explicitly added the check for pointers into kernel
text).

I'm just trying to explain as fully as possible what the resulting
code does and why.

> it's not pointer validation but bounds checking: you already know which memory
> object the pointer is supposed to point to, you only check its bounds. if it was
> an attacker controlled pointer then all this would be a pointless check of course,
> trivial for an attacker to circumvent (and this is why it's not part of the
> USERCOPY design).

Agreed: but the pointer is being checked to attempt to figure out what
KIND of object is being copied. It is part of the logic. If it helps
people understand it more clearly, I can describe them as separate
steps: identify the object type, then perform bounds checking of the
size on that type.

>> > yes, more bikeshedding will surely help, [...]
>>
>> Insulting and ridiculing a reviewer who explicitly qualified his comments with
>> "one minor nit to pick" sure does not help upstream integration either.
>
> sorry Ingo, but calling a spade a spade isn't insulting, at best it's exposing
> some painful truth. you yourself used that term several times in the past, were
> you insulting and ridiculing people then?
>
> as for the ad hominem that you displayed here and later, i hope that in the
> future you will display the same professional conduct that you apparently expect
> from others.

There's a long history of misunderstanding and miscommunication
(intentional or otherwise) by everyone on these topics. I'd love it if
we can just side-step all of it, and try to stick as closely to the
technical discussions as possible. Everyone involved in these
discussions wants better security, even if we go about it in different
ways. If anyone finds themselves feeling insulted, just try to let it
go, and focus on the places where we can find productive common
ground, remembering that any fighting just distracts from the more
important issues at hand.

> i'll voice my opinion when you guys are about to screw it up (as it happened in
> the past and apparently history keeps repeating itself). if you don't want my
> opinion then don't ask for it (in that case we'll write a blog at most ;).

I am hugely interested in your involvement in these discussions:
you're by far the most knowledgeable about them. You generally give
very productive feedback, and for that I'm thankful. I prefer that to
just saying something is wrong/broken without any actionable
follow-up. :)

>> > [...] 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.

Part of getting protections into upstream is doing them in ways that
make them palatable for incremental work. As it happened, the
read-after-init piece of the larger read-only attack surface reduction
effort was small enough to make it in. As more work is done, we can
continue to build on it.

Making rodata read-only before mark_rodata() is part of my longer goal
since other architectures (e.g. s390) already do this, and is
technically the more correct thing to do: rodata should start its life
read-only. It's a weird hack that it is delayed at all.

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

On the surface, I don't disagree, but as upstream is a large-scale
collaborative effort, I tend to focus on what things are specifically
critical, and naming isn't one of them. :)

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

Right, and I think part of this is a failure of documentation and
examples. As we make progress with REFCOUNT, we can learn about the
best way to approach these kinds of larger tree-wide changes under the
constraints of the existing upstream development process.

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

Do you mean the rest of the KERNEXEC (hopefully I'm not confusing
implementation names) code that uses pax_open/close_kernel()? I expect
that to be a gradual addition too, and I'd love participation to get
it and the constify plugin into the kernel.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security