On Sun, Jul 10, 2016 at 8:38 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote: > On Sun, Jul 10, 2016 at 5:03 AM, PaX Team <pageexec@xxxxxxxxxxx> wrote: >> On 10 Jul 2016 at 11:16, Ingo Molnar wrote: >> >>> * PaX Team <pageexec@xxxxxxxxxxx> 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. Well, the CONFIG_HAVE* stuff is almost entirely invisible to the end-user, and I feel like it's better to be specific about names now, and when they change their meaning, we can change their names with it. I intend to extend the HARDENED_USERCOPY logic in similar ways to how it is extended in Grsecurity: parts can be used for the "is this destined for a userspace memory buffer?" test when rejecting writing pointers or other sensitive information during sprintf (see the HIDESYM work in grsecurity). But, I don't like to over-think it: right now, it is named for what it does, and we can adjust as we need to. > >>> > [...] 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. I expect that if/when we add the open/close_kernel logic, we'll have a new section and it will be named accordingly (since it, too, is not const-in-the-C-sense, and shouldn't live in the standard .rodata section). > (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.) -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>