lkml.org 
[lkml]   [2016]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 0/9] mm: Hardened usercopy
    On Sun, Jul 10, 2016 at 5:03 AM, PaX Team <pageexec@freemail.hu> wrote:
    > On 10 Jul 2016 at 11:16, Ingo Molnar wrote:
    >
    >> * PaX Team <pageexec@freemail.hu> wrote:
    >>
    >> > On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:
    >> >
    >> > > 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.

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

    \
     
     \ /
      Last update: 2016-07-10 15:21    [W:2.665 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site