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

[Kees Cook]

On Sun, Jul 10, 2016 at 8:38 AM, Andy Lutomirski  wrote:
> On Sun, Jul 10, 2016 at 5:03 AM, PaX Team  wrote:
>> On 10 Jul 2016 at 11:16, Ingo Molnar wrote:
>>
>>> * PaX Team  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 

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

[Kees Cook]

On Sun, Jul 10, 2016 at 8:03 AM, PaX Team  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.

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

[Andy Lutomirski]

On Sun, Jul 10, 2016 at 5:03 AM, PaX Team  wrote:
> On 10 Jul 2016 at 11:16, Ingo Molnar wrote:
>
>> * PaX Team  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 

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

[PaX Team]

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

> * PaX Team  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 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).

> There are a number of cheap tests we can do and there are a number of ways 
> how a 
> 'pointer' can be validated runtime, without any 'size' information:
> 
>  - for example if a pointer points into a red zone straight away then we know 
> it's
>bogus.

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

>  - or if a kernel pointer is points outside the valid kernel virtual memory 
> range
>we know it's bogus as well.

accesses outside of valid virtual memory will cause a page fault ('oops' in 
linux
terms), there's no need to explicitly check for that.

> So while only doing a bounds check might have been the original purpose of 
> the 
> patch set, Andy's point is that it might make sense to treat this facility as 
> a 
> more generic 'object validation' code of (pointer,size) object and not limit 
> it to 
> 'runtime bounds checking'.

FYI, 'runtime bounds checking' is a terminus technicus and it is about 
validating
both the pointer and underlying object's size. that's the reason i called 
USERCOPY
a 'very specific form' of it only since it doesn't validate each part equally 
well
(or well enough at all, even the size check is not as precise as it could be).

as for what does or doesn't make sense, first you'll have to define a threat
model and evaluate everything else based on that. since noone has solved the
general bounds checking problem with acceptable properties (mostly performance
impact, but also memory overhead, etc), i'm all ears to hear what you guys have
come up with.

> That kind of extended purpose behind a facility should be reflected in the 
> naming.
> Confusing names are often the source of misunderstandings and bugs.

definitely, but before you bikeshed on naming, you should figure out what and 
why
you want to do, whether it's even feasible, meaningful, useful, etc. answering 
the
opening question and digging into the details is the first step of any design
process, not its naming.

> The 9-patch series as submitted here is neither just 'bounds checking' nor 
> just 
> pure 'pointer checking', it's about validating that a (pointer,size) range of 
> memory passed to a (user) memory copy function is fully within a valid object 
> the 
> kernel might know about (in an fast to check fashion).
> 
> This necessary means:
> 
>  - the start of the range points to a valid object to begin 

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

[Ingo Molnar]

* PaX Team  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.

There are a number of cheap tests we can do and there are a number of ways how 
a 
'pointer' can be validated runtime, without any 'size' information:

 - for example if a pointer points into a red zone straight away then we know 
it's
   bogus.

 - or if a kernel pointer is points outside the valid kernel virtual memory 
range
   we know it's bogus as well.

So while only doing a bounds check might have been the original purpose of the 
patch set, Andy's point is that it might make sense to treat this facility as a 
more generic 'object validation' code of (pointer,size) object and not limit it 
to 
'runtime bounds checking'. That kind of extended purpose behind a facility 
should 
be reflected in the naming.

Confusing names are often the source of misunderstandings and bugs.

The 9-patch series as submitted here is neither just 'bounds checking' nor just 
pure 'pointer checking', it's about validating that a (pointer,size) range of 
memory passed to a (user) memory copy function is fully within a valid object 
the 
kernel might know about (in an fast to check fashion).

This necessary means:

 - the start of the range points to a valid object to begin with (if known)

 - the range itself does not point beyond the end of the object (if known)

 - even if the kernel does not know anything about the pointed to object it can 
   do a pointer check (for example is it pointing inside kernel virtual memory) 
   and do a bounds check on the size.

Do you disagree with that?

> > Might it make sense to call the infrastructure part something else?
> 
> 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. (Unless 
the goal is to prevent upstream integration.)

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

Names within submitted patches get renamed on a routine basis during review. 
It's 
often only minor improvements in naming (which you can consider bike shedding), 
but in this particular case the rename was clearly useful in not just improving 
the name but in avoiding an actively confusing name. So I disagree not just 
with 
the hostile tone of your reply but with your underlying technical point as well.

Thanks,

Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[PaX Team]

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

> On Jul 6, 2016 6:25 PM, "Kees Cook"  wrote:
> >
> > Hi,
> >
> > This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> > writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> > kept tweaking things further and further until I ended up with a whole
> > new patch series. To that end, I took Rik's feedback and made a number
> > of other changes and clean-ups as well.
> >
> 
> 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. it's only really relevant for slab objects and the
pointer checks (that one might mistake for being a part of the defense 
mechanism)
are only there to determine whether the kernel pointer refers to a slab object
or not (the stack part is a small bonus and was never the main goal either).

> Might it make sense to call the infrastructure part something else?

yes, more bikeshedding will surely help, 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...

> After all, this could be extended in the future for memcpy or even for
> some GCC plugin to check pointers passed to ordinary (non-allocator)
> functions.

what kind of checks are you thinking of here? and more fundamentally, against
what kind of threats? as for memcpy, it's the standard mandated memory copying
function, what security related properties can it check on its pointer 
arguments?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Andy Lutomirski]

On Jul 6, 2016 6:25 PM, "Kees Cook"  wrote:
>
> Hi,
>
> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> kept tweaking things further and further until I ended up with a whole
> new patch series. To that end, I took Rik's feedback and made a number
> of other changes and clean-ups as well.
>

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.
Might it make sense to call the infrastructure part something else?
After all, this could be extended in the future for memcpy or even for
some GCC plugin to check pointers passed to ordinary (non-allocator)
functions.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Kees Cook]

On Sat, Jul 9, 2016 at 1:25 AM, Ard Biesheuvel
 wrote:
> On 9 July 2016 at 04:22, Laura Abbott  wrote:
>> On 07/06/2016 03:25 PM, Kees Cook wrote:
>>>
>>> Hi,
>>>
>>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>>> kept tweaking things further and further until I ended up with a whole
>>> new patch series. To that end, I took Rik's feedback and made a number
>>> of other changes and clean-ups as well.
>>>
>>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>>> changes don't touch get_user() and put_user(), since these operate on
>>> constant sized lengths, and tend to be much less vulnerable. There
>>> are effectively three distinct protections in the whole series,
>>> each of which I've given a separate CONFIG, though this patch set is
>>> only the first of the three intended protections. (Generally speaking,
>>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>>
>>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>>> being copied to/from userspace meet certain criteria:
>>> - if address is a heap object, the size must not exceed the object's
>>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>>> - if address range is in the current process stack, it must be within the
>>>   current stack frame (if such checking is possible) or at least entirely
>>>   within the current process's stack. (This could catch large lengths that
>>>   would have extended beyond the current process stack, or overflows if
>>>   their length extends back into the original stack.)
>>> - if the address range is part of kernel data, rodata, or bss, allow it.
>>> - if address range is page-allocated, that it doesn't span multiple
>>>   allocations.
>>> - if address is within the kernel text, reject it.
>>> - everything else is accepted
>>>
>>> The patches in the series are:
>>> - The core copy_to/from_user() checks, without the slab object checks:
>>> 1- mm: Hardened usercopy
>>> - Per-arch enablement of the protection:
>>> 2- x86/uaccess: Enable hardened usercopy
>>> 3- ARM: uaccess: Enable hardened usercopy
>>> 4- arm64/uaccess: Enable hardened usercopy
>>> 5- ia64/uaccess: Enable hardened usercopy
>>> 6- powerpc/uaccess: Enable hardened usercopy
>>> 7- sparc/uaccess: Enable hardened usercopy
>>> - The heap allocator implementation of object size checking:
>>> 8- mm: SLAB hardened usercopy support
>>> 9- mm: SLUB hardened usercopy support
>>>
>>> Some notes:
>>>
>>> - This is expected to apply on top of -next which contains fixes for the
>>>   position of _etext on both arm and arm64.
>>>
>>> - I couldn't detect a measurable performance change with these features
>>>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>>>   etc. I think we could flip this to "on by default" at some point.
>>>
>>> - The SLOB support extracted from grsecurity seems entirely broken. I
>>>   have no idea what's going on there, I spent my time testing SLAB and
>>>   SLUB. Having someone else look at SLOB would be nice, but this series
>>>   doesn't depend on it.
>>>
>>> Additional features that would be nice, but aren't blocking this series:
>>>
>>> - Needs more architecture support for stack frame checking (only x86 now).
>>>
>>>
>>
>> Even with the SLUB fixup I'm still seeing this blow up on my arm64 system.
>> This is a
>> Fedora rawhide kernel + the patches
>>
>> [ 0.666700] usercopy: kernel memory exposure attempt detected from
>> fc0008b4dd58 () (8 bytes)
>> [ 0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: GW
>> 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
>> [ 0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov 24
>> 2015
>> [ 0.666744] Call trace:
>> [ 0.666756] [] dump_backtrace+0x0/0x1e8
>> [ 0.666765] [] show_stack+0x24/0x30
>> [ 0.666775] [] dump_stack+0xa4/0xe0
>> [ 0.666785] [] __check_object_size+0x6c/0x230
>> [ 0.666795] [] create_elf_tables+0x74/0x420
>> [ 0.666805] [] load_elf_binary+0x828/0xb70
>> [ 0.666814] [] search_binary_handler+0xb4/0x240
>> [ 0.666823] [] do_execveat_common+0x63c/0x950
>> [ 0.666832] [] do_execve+0x3c/0x50
>> [ 0.666841] [] call_usermodehelper_exec_async+0xe8/0x148
>> [ 0.666850] [] ret_from_fork+0x10/0x50
>>
>> This happens on every call to execve. This seems to be the first
>> copy_to_user in
>> create_elf_tables. I didn't get a chance to debug and I'm going out of town
>> all of next week so all I have is the report unfortunately. config attached.
>>
>
> This is a known issue, and a fix is already queued for v4.8 in the arm64 tree:
>
> 

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

[Kees Cook]

On Fri, Jul 8, 2016 at 7:22 PM, Laura Abbott  wrote:
> On 07/06/2016 03:25 PM, Kees Cook wrote:
>>
>> Hi,
>>
>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>> kept tweaking things further and further until I ended up with a whole
>> new patch series. To that end, I took Rik's feedback and made a number
>> of other changes and clean-ups as well.
>>
>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>> changes don't touch get_user() and put_user(), since these operate on
>> constant sized lengths, and tend to be much less vulnerable. There
>> are effectively three distinct protections in the whole series,
>> each of which I've given a separate CONFIG, though this patch set is
>> only the first of the three intended protections. (Generally speaking,
>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>
>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>> being copied to/from userspace meet certain criteria:
>> - if address is a heap object, the size must not exceed the object's
>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack. (This could catch large lengths that
>>   would have extended beyond the current process stack, or overflows if
>>   their length extends back into the original stack.)
>> - if the address range is part of kernel data, rodata, or bss, allow it.
>> - if address range is page-allocated, that it doesn't span multiple
>>   allocations.
>> - if address is within the kernel text, reject it.
>> - everything else is accepted
>>
>> The patches in the series are:
>> - The core copy_to/from_user() checks, without the slab object checks:
>> 1- mm: Hardened usercopy
>> - Per-arch enablement of the protection:
>> 2- x86/uaccess: Enable hardened usercopy
>> 3- ARM: uaccess: Enable hardened usercopy
>> 4- arm64/uaccess: Enable hardened usercopy
>> 5- ia64/uaccess: Enable hardened usercopy
>> 6- powerpc/uaccess: Enable hardened usercopy
>> 7- sparc/uaccess: Enable hardened usercopy
>> - The heap allocator implementation of object size checking:
>> 8- mm: SLAB hardened usercopy support
>> 9- mm: SLUB hardened usercopy support
>>
>> Some notes:
>>
>> - This is expected to apply on top of -next which contains fixes for the
>>   position of _etext on both arm and arm64.
>>
>> - I couldn't detect a measurable performance change with these features
>>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>>   etc. I think we could flip this to "on by default" at some point.
>>
>> - The SLOB support extracted from grsecurity seems entirely broken. I
>>   have no idea what's going on there, I spent my time testing SLAB and
>>   SLUB. Having someone else look at SLOB would be nice, but this series
>>   doesn't depend on it.
>>
>> Additional features that would be nice, but aren't blocking this series:
>>
>> - Needs more architecture support for stack frame checking (only x86 now).
>>
>>
>
> Even with the SLUB fixup I'm still seeing this blow up on my arm64 system.
> This is a
> Fedora rawhide kernel + the patches

Is this on top of -next? The recent _etext change ("arm64: mm: fix
location of _etext") is needed to fix the kernel text test for arm64.

-Kees

>
> [0.666700] usercopy: kernel memory exposure attempt detected from
> fc0008b4dd58 () (8 bytes)
> [0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: GW
> 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> [0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov
> 24 2015
> [0.666744] Call trace:
> [0.666756] [] dump_backtrace+0x0/0x1e8
> [0.666765] [] show_stack+0x24/0x30
> [0.666775] [] dump_stack+0xa4/0xe0
> [0.666785] [] __check_object_size+0x6c/0x230
> [0.666795] [] create_elf_tables+0x74/0x420
> [0.666805] [] load_elf_binary+0x828/0xb70
> [0.666814] [] search_binary_handler+0xb4/0x240
> [0.666823] [] do_execveat_common+0x63c/0x950
> [0.666832] [] do_execve+0x3c/0x50
> [0.666841] []
> call_usermodehelper_exec_async+0xe8/0x148
> [0.666850] [] ret_from_fork+0x10/0x50
>
> This happens on every call to execve. This seems to be the first
> copy_to_user in
> create_elf_tables. I didn't get a chance to debug and I'm going out of town
> all of next week so all I have is the report unfortunately. config attached.
>
> Thanks,
> Laura



-- 
Kees Cook
Chrome OS & Brillo Security

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

[Laura Abbott]

On Sat, Jul 9, 2016 at 1:25 AM, Ard Biesheuvel 
wrote:

> On 9 July 2016 at 04:22, Laura Abbott  wrote:
> > On 07/06/2016 03:25 PM, Kees Cook wrote:
> >>
> >> Hi,
> >>
> >> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> >> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> >> kept tweaking things further and further until I ended up with a whole
> >> new patch series. To that end, I took Rik's feedback and made a number
> >> of other changes and clean-ups as well.
> >>
> >> Based on my understanding, PAX_USERCOPY was designed to catch a few
> >> classes of flaws around the use of copy_to_user()/copy_from_user().
> These
> >> changes don't touch get_user() and put_user(), since these operate on
> >> constant sized lengths, and tend to be much less vulnerable. There
> >> are effectively three distinct protections in the whole series,
> >> each of which I've given a separate CONFIG, though this patch set is
> >> only the first of the three intended protections. (Generally speaking,
> >> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
> >> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS
> covers
> >> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
> >>
> >> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
> >> being copied to/from userspace meet certain criteria:
> >> - if address is a heap object, the size must not exceed the object's
> >>   allocated size. (This will catch all kinds of heap overflow flaws.)
> >> - if address range is in the current process stack, it must be within
> the
> >>   current stack frame (if such checking is possible) or at least
> entirely
> >>   within the current process's stack. (This could catch large lengths
> that
> >>   would have extended beyond the current process stack, or overflows if
> >>   their length extends back into the original stack.)
> >> - if the address range is part of kernel data, rodata, or bss, allow it.
> >> - if address range is page-allocated, that it doesn't span multiple
> >>   allocations.
> >> - if address is within the kernel text, reject it.
> >> - everything else is accepted
> >>
> >> The patches in the series are:
> >> - The core copy_to/from_user() checks, without the slab object checks:
> >> 1- mm: Hardened usercopy
> >> - Per-arch enablement of the protection:
> >> 2- x86/uaccess: Enable hardened usercopy
> >> 3- ARM: uaccess: Enable hardened usercopy
> >> 4- arm64/uaccess: Enable hardened usercopy
> >> 5- ia64/uaccess: Enable hardened usercopy
> >> 6- powerpc/uaccess: Enable hardened usercopy
> >> 7- sparc/uaccess: Enable hardened usercopy
> >> - The heap allocator implementation of object size checking:
> >> 8- mm: SLAB hardened usercopy support
> >> 9- mm: SLUB hardened usercopy support
> >>
> >> Some notes:
> >>
> >> - This is expected to apply on top of -next which contains fixes for the
> >>   position of _etext on both arm and arm64.
> >>
> >> - I couldn't detect a measurable performance change with these features
> >>   enabled. Kernel build times were unchanged, hackbench was unchanged,
> >>   etc. I think we could flip this to "on by default" at some point.
> >>
> >> - The SLOB support extracted from grsecurity seems entirely broken. I
> >>   have no idea what's going on there, I spent my time testing SLAB and
> >>   SLUB. Having someone else look at SLOB would be nice, but this series
> >>   doesn't depend on it.
> >>
> >> Additional features that would be nice, but aren't blocking this series:
> >>
> >> - Needs more architecture support for stack frame checking (only x86
> now).
> >>
> >>
> >
> > Even with the SLUB fixup I'm still seeing this blow up on my arm64
> system.
> > This is a
> > Fedora rawhide kernel + the patches
> >
> > [ 0.666700] usercopy: kernel memory exposure attempt detected from
> > fc0008b4dd58 () (8 bytes)
> > [ 0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: GW
> > 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> > [ 0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov
> 24
> > 2015
> > [ 0.666744] Call trace:
> > [ 0.666756] [] dump_backtrace+0x0/0x1e8
> > [ 0.666765] [] show_stack+0x24/0x30
> > [ 0.666775] [] dump_stack+0xa4/0xe0
> > [ 0.666785] [] __check_object_size+0x6c/0x230
> > [ 0.666795] [] create_elf_tables+0x74/0x420
> > [ 0.666805] [] load_elf_binary+0x828/0xb70
> > [ 0.666814] [] search_binary_handler+0xb4/0x240
> > [ 0.666823] [] do_execveat_common+0x63c/0x950
> > [ 0.666832] [] do_execve+0x3c/0x50
> > [ 0.666841] []
> call_usermodehelper_exec_async+0xe8/0x148
> > [ 0.666850] [] ret_from_fork+0x10/0x50
> >
> > This happens on every call to execve. This seems to be the first
> > copy_to_user in
> > create_elf_tables. I didn't get a chance to debug and I'm going out of
> town
> > all of next week so all I have is the report 

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

[Ard Biesheuvel]

On 9 July 2016 at 04:22, Laura Abbott  wrote:
> On 07/06/2016 03:25 PM, Kees Cook wrote:
>>
>> Hi,
>>
>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>> kept tweaking things further and further until I ended up with a whole
>> new patch series. To that end, I took Rik's feedback and made a number
>> of other changes and clean-ups as well.
>>
>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>> changes don't touch get_user() and put_user(), since these operate on
>> constant sized lengths, and tend to be much less vulnerable. There
>> are effectively three distinct protections in the whole series,
>> each of which I've given a separate CONFIG, though this patch set is
>> only the first of the three intended protections. (Generally speaking,
>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>
>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>> being copied to/from userspace meet certain criteria:
>> - if address is a heap object, the size must not exceed the object's
>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack. (This could catch large lengths that
>>   would have extended beyond the current process stack, or overflows if
>>   their length extends back into the original stack.)
>> - if the address range is part of kernel data, rodata, or bss, allow it.
>> - if address range is page-allocated, that it doesn't span multiple
>>   allocations.
>> - if address is within the kernel text, reject it.
>> - everything else is accepted
>>
>> The patches in the series are:
>> - The core copy_to/from_user() checks, without the slab object checks:
>> 1- mm: Hardened usercopy
>> - Per-arch enablement of the protection:
>> 2- x86/uaccess: Enable hardened usercopy
>> 3- ARM: uaccess: Enable hardened usercopy
>> 4- arm64/uaccess: Enable hardened usercopy
>> 5- ia64/uaccess: Enable hardened usercopy
>> 6- powerpc/uaccess: Enable hardened usercopy
>> 7- sparc/uaccess: Enable hardened usercopy
>> - The heap allocator implementation of object size checking:
>> 8- mm: SLAB hardened usercopy support
>> 9- mm: SLUB hardened usercopy support
>>
>> Some notes:
>>
>> - This is expected to apply on top of -next which contains fixes for the
>>   position of _etext on both arm and arm64.
>>
>> - I couldn't detect a measurable performance change with these features
>>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>>   etc. I think we could flip this to "on by default" at some point.
>>
>> - The SLOB support extracted from grsecurity seems entirely broken. I
>>   have no idea what's going on there, I spent my time testing SLAB and
>>   SLUB. Having someone else look at SLOB would be nice, but this series
>>   doesn't depend on it.
>>
>> Additional features that would be nice, but aren't blocking this series:
>>
>> - Needs more architecture support for stack frame checking (only x86 now).
>>
>>
>
> Even with the SLUB fixup I'm still seeing this blow up on my arm64 system.
> This is a
> Fedora rawhide kernel + the patches
>
> [ 0.666700] usercopy: kernel memory exposure attempt detected from
> fc0008b4dd58 () (8 bytes)
> [ 0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: GW
> 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> [ 0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov 24
> 2015
> [ 0.666744] Call trace:
> [ 0.666756] [] dump_backtrace+0x0/0x1e8
> [ 0.666765] [] show_stack+0x24/0x30
> [ 0.666775] [] dump_stack+0xa4/0xe0
> [ 0.666785] [] __check_object_size+0x6c/0x230
> [ 0.666795] [] create_elf_tables+0x74/0x420
> [ 0.666805] [] load_elf_binary+0x828/0xb70
> [ 0.666814] [] search_binary_handler+0xb4/0x240
> [ 0.666823] [] do_execveat_common+0x63c/0x950
> [ 0.666832] [] do_execve+0x3c/0x50
> [ 0.666841] [] call_usermodehelper_exec_async+0xe8/0x148
> [ 0.666850] [] ret_from_fork+0x10/0x50
>
> This happens on every call to execve. This seems to be the first
> copy_to_user in
> create_elf_tables. I didn't get a chance to debug and I'm going out of town
> all of next week so all I have is the report unfortunately. config attached.
>

This is a known issue, and a fix is already queued for v4.8 in the arm64 tree:

9fdc14c55c arm64: mm: fix location of _etext [0]

which moves _etext up in the linker script so that it does not cover .rodata

ARM was suffering from the same problem, and Kees proposed a 

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

[Ingo Molnar]

* Rik van Riel  wrote:

> On Fri, 2016-07-08 at 19:22 -0700, Laura Abbott wrote:
> > 
> > Even with the SLUB fixup I'm still seeing this blow up on my arm64
> > system. This is a
> > Fedora rawhide kernel + the patches
> > 
> > [0.666700] usercopy: kernel memory exposure attempt detected from
> > fc0008b4dd58 () (8 bytes)
> > [0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted:
> > GW   4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> > [0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS
> > 1.1.0 Nov 24 2015
> > [0.666744] Call trace:
> > [0.666756] [] dump_backtrace+0x0/0x1e8
> > [0.666765] [] show_stack+0x24/0x30
> > [0.666775] [] dump_stack+0xa4/0xe0
> > [0.666785] [] __check_object_size+0x6c/0x230
> > [0.666795] [] create_elf_tables+0x74/0x420
> > [0.666805] [] load_elf_binary+0x828/0xb70
> > [0.666814] [] search_binary_handler+0xb4/0x240
> > [0.666823] [] do_execveat_common+0x63c/0x950
> > [0.666832] [] do_execve+0x3c/0x50
> > [0.666841] []
> > call_usermodehelper_exec_async+0xe8/0x148
> > [0.666850] [] ret_from_fork+0x10/0x50
> > 
> > This happens on every call to execve. This seems to be the first
> > copy_to_user in
> > create_elf_tables. I didn't get a chance to debug and I'm going out
> > of town
> > all of next week so all I have is the report unfortunately. config
> > attached.
> 
> That's odd, this should be copying a piece of kernel data (not text)
> to userspace.
> 
> from fs/binfmt_elf.c
> 
>         const char *k_platform = ELF_PLATFORM;
> 
> ...
>                 size_t len = strlen(k_platform) + 1;
>   
>                 u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
> if (__copy_to_user(u_platform, k_platform, len))
> return -EFAULT;
> 
> from arch/arm/include/asm/elf.h:
> 
> #define ELF_PLATFORM_SIZE 8
> #define ELF_PLATFORM(elf_platform)
> 
> extern char elf_platform[];
> 
> from arch/arm/kernel/setup.c:
> 
> char elf_platform[ELF_PLATFORM_SIZE];
> EXPORT_SYMBOL(elf_platform);
> 
> ...
> 
> snprintf(elf_platform, ELF_PLATFORM_SIZE, "%s%c",
>  list->elf_name, ENDIANNESS);
> 
> How does that end up in the .text section of the
> image, instead of in one of the various data sections?
> 
> What kind of linker oddity is going on with ARM?

I think the crash happened on ARM64, not ARM.

Thanks,

Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Rik van Riel]

On Fri, 2016-07-08 at 19:22 -0700, Laura Abbott wrote:
> 
> Even with the SLUB fixup I'm still seeing this blow up on my arm64
> system. This is a
> Fedora rawhide kernel + the patches
> 
> [0.666700] usercopy: kernel memory exposure attempt detected from
> fc0008b4dd58 () (8 bytes)
> [0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted:
> GW   4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> [0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS
> 1.1.0 Nov 24 2015
> [0.666744] Call trace:
> [0.666756] [] dump_backtrace+0x0/0x1e8
> [0.666765] [] show_stack+0x24/0x30
> [0.666775] [] dump_stack+0xa4/0xe0
> [0.666785] [] __check_object_size+0x6c/0x230
> [0.666795] [] create_elf_tables+0x74/0x420
> [0.666805] [] load_elf_binary+0x828/0xb70
> [0.666814] [] search_binary_handler+0xb4/0x240
> [0.666823] [] do_execveat_common+0x63c/0x950
> [0.666832] [] do_execve+0x3c/0x50
> [0.666841] []
> call_usermodehelper_exec_async+0xe8/0x148
> [0.666850] [] ret_from_fork+0x10/0x50
> 
> This happens on every call to execve. This seems to be the first
> copy_to_user in
> create_elf_tables. I didn't get a chance to debug and I'm going out
> of town
> all of next week so all I have is the report unfortunately. config
> attached.

That's odd, this should be copying a piece of kernel data (not text)
to userspace.

from fs/binfmt_elf.c

        const char *k_platform = ELF_PLATFORM;

...
                size_t len = strlen(k_platform) + 1;

                u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
if (__copy_to_user(u_platform, k_platform, len))
return -EFAULT;

from arch/arm/include/asm/elf.h:

#define ELF_PLATFORM_SIZE 8
#define ELF_PLATFORM(elf_platform)

extern char elf_platform[];

from arch/arm/kernel/setup.c:

char elf_platform[ELF_PLATFORM_SIZE];
EXPORT_SYMBOL(elf_platform);

...

snprintf(elf_platform, ELF_PLATFORM_SIZE, "%s%c",
 list->elf_name, ENDIANNESS);

How does that end up in the .text section of the
image, instead of in one of the various data sections?

What kind of linker oddity is going on with ARM?

--  
All Rights Reversed.

signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Ingo Molnar]

* Linus Torvalds  wrote:

> On Fri, Jul 8, 2016 at 1:46 AM, Ingo Molnar  wrote:
> >
> > Could you please try to find some syscall workload that does many small user
> > copies and thus excercises this code path aggressively?
> 
> Any stat()-heavy path will hit cp_new_stat() very heavily. Think the
> usual kind of "traverse the whole tree looking for something". "git
> diff" will do it, just checking that everything is up-to-date.
> 
> That said, other things tend to dominate.

So I think a cached 'find /usr >/dev/null' might be a good one as well:

 triton:~/tip> strace -c find /usr >/dev/null
 % time seconds  usecs/call callserrors syscall
 -- --- --- - - 
  47.090.006518   0254697   newfstatat
  26.200.003627   0254795   getdents
  14.450.002000   0   1147411   fcntl
   7.330.001014   0509811   close
   3.280.000454   0128220 1 openat
   1.520.000210   0128230   fstat
   0.270.16   0 12810   write
   0.000.00   010   read

 triton:~/tip> perf stat --repeat 3 -e cycles:u,cycles:k,cycles find /usr 
>/dev/null

 Performance counter stats for 'find /usr' (3 runs):

 1,594,437,143  cycles:u
  ( +-  2.76% )
 2,570,544,009  cycles:k
  ( +-  2.50% )
 4,164,981,152  cycles  
  ( +-  2.59% )

   0.929883686 seconds time elapsed 
 ( +-  2.57% )

... and it's dominated by kernel overhead, with a fair amount of memcpy 
overhead 
as well:

   1.22%  find [kernel.kallsyms]   [k] copy_user_enhanced_fast_string   

 

But maybe there are simple shell commands that are even more user-memcpy 
intense? 

Thanks,

Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Linus Torvalds]

On Fri, Jul 8, 2016 at 1:46 AM, Ingo Molnar  wrote:
>
> Could you please try to find some syscall workload that does many small user
> copies and thus excercises this code path aggressively?

Any stat()-heavy path will hit cp_new_stat() very heavily. Think the
usual kind of "traverse the whole tree looking for something". "git
diff" will do it, just checking that everything is up-to-date.

That said, other things tend to dominate.

 Linus
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Ingo Molnar]

* Kees Cook  wrote:

> - I couldn't detect a measurable performance change with these features
>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>   etc. I think we could flip this to "on by default" at some point.

Could you please try to find some syscall workload that does many small user 
copies and thus excercises this code path aggressively?

If that measurement works out fine then I'd prefer to enable these security 
checks 
by default.

Thaks,

Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Kees Cook]

On Thu, Jul 7, 2016 at 3:30 AM, Christian Borntraeger
 wrote:
> On 07/07/2016 12:25 AM, Kees Cook wrote:
>> Hi,
>>
>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>> kept tweaking things further and further until I ended up with a whole
>> new patch series. To that end, I took Rik's feedback and made a number
>> of other changes and clean-ups as well.
>>
>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>> changes don't touch get_user() and put_user(), since these operate on
>> constant sized lengths, and tend to be much less vulnerable. There
>> are effectively three distinct protections in the whole series,
>> each of which I've given a separate CONFIG, though this patch set is
>> only the first of the three intended protections. (Generally speaking,
>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>
>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>> being copied to/from userspace meet certain criteria:
>> - if address is a heap object, the size must not exceed the object's
>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack. (This could catch large lengths that
>>   would have extended beyond the current process stack, or overflows if
>>   their length extends back into the original stack.)
>> - if the address range is part of kernel data, rodata, or bss, allow it.
>> - if address range is page-allocated, that it doesn't span multiple
>>   allocations.
>> - if address is within the kernel text, reject it.
>> - everything else is accepted
>>
>> The patches in the series are:
>> - The core copy_to/from_user() checks, without the slab object checks:
>>   1- mm: Hardened usercopy
>> - Per-arch enablement of the protection:
>>   2- x86/uaccess: Enable hardened usercopy
>>   3- ARM: uaccess: Enable hardened usercopy
>>   4- arm64/uaccess: Enable hardened usercopy
>>   5- ia64/uaccess: Enable hardened usercopy
>>   6- powerpc/uaccess: Enable hardened usercopy
>>   7- sparc/uaccess: Enable hardened usercopy
>
> Was there a reason why you did not change s390?

No reason -- just didn't have a good build setup for testing it.
(Everything but arm64 was already in grsecurity, and I was able to
build-test arm64 when I added it there.) I would love to include s390
too!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Christian Borntraeger]

On 07/07/2016 12:25 AM, Kees Cook wrote:
> Hi,
> 
> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> kept tweaking things further and further until I ended up with a whole
> new patch series. To that end, I took Rik's feedback and made a number
> of other changes and clean-ups as well.
> 
> Based on my understanding, PAX_USERCOPY was designed to catch a few
> classes of flaws around the use of copy_to_user()/copy_from_user(). These
> changes don't touch get_user() and put_user(), since these operate on
> constant sized lengths, and tend to be much less vulnerable. There
> are effectively three distinct protections in the whole series,
> each of which I've given a separate CONFIG, though this patch set is
> only the first of the three intended protections. (Generally speaking,
> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
> 
> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
> being copied to/from userspace meet certain criteria:
> - if address is a heap object, the size must not exceed the object's
>   allocated size. (This will catch all kinds of heap overflow flaws.)
> - if address range is in the current process stack, it must be within the
>   current stack frame (if such checking is possible) or at least entirely
>   within the current process's stack. (This could catch large lengths that
>   would have extended beyond the current process stack, or overflows if
>   their length extends back into the original stack.)
> - if the address range is part of kernel data, rodata, or bss, allow it.
> - if address range is page-allocated, that it doesn't span multiple
>   allocations.
> - if address is within the kernel text, reject it.
> - everything else is accepted
> 
> The patches in the series are:
> - The core copy_to/from_user() checks, without the slab object checks:
>   1- mm: Hardened usercopy
> - Per-arch enablement of the protection:
>   2- x86/uaccess: Enable hardened usercopy
>   3- ARM: uaccess: Enable hardened usercopy
>   4- arm64/uaccess: Enable hardened usercopy
>   5- ia64/uaccess: Enable hardened usercopy
>   6- powerpc/uaccess: Enable hardened usercopy
>   7- sparc/uaccess: Enable hardened usercopy

Was there a reason why you did not change s390?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

Hi,

This is a start of the mainline port of PAX_USERCOPY[1]. After I started
writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
kept tweaking things further and further until I ended up with a whole
new patch series. To that end, I took Rik's feedback and made a number
of other changes and clean-ups as well.

Based on my understanding, PAX_USERCOPY was designed to catch a few
classes of flaws around the use of copy_to_user()/copy_from_user(). These
changes don't touch get_user() and put_user(), since these operate on
constant sized lengths, and tend to be much less vulnerable. There
are effectively three distinct protections in the whole series,
each of which I've given a separate CONFIG, though this patch set is
only the first of the three intended protections. (Generally speaking,
PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)

This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
being copied to/from userspace meet certain criteria:
- if address is a heap object, the size must not exceed the object's
  allocated size. (This will catch all kinds of heap overflow flaws.)
- if address range is in the current process stack, it must be within the
  current stack frame (if such checking is possible) or at least entirely
  within the current process's stack. (This could catch large lengths that
  would have extended beyond the current process stack, or overflows if
  their length extends back into the original stack.)
- if the address range is part of kernel data, rodata, or bss, allow it.
- if address range is page-allocated, that it doesn't span multiple
  allocations.
- if address is within the kernel text, reject it.
- everything else is accepted

The patches in the series are:
- The core copy_to/from_user() checks, without the slab object checks:
1- mm: Hardened usercopy
- Per-arch enablement of the protection:
2- x86/uaccess: Enable hardened usercopy
3- ARM: uaccess: Enable hardened usercopy
4- arm64/uaccess: Enable hardened usercopy
5- ia64/uaccess: Enable hardened usercopy
6- powerpc/uaccess: Enable hardened usercopy
7- sparc/uaccess: Enable hardened usercopy
- The heap allocator implementation of object size checking:
8- mm: SLAB hardened usercopy support
9- mm: SLUB hardened usercopy support

Some notes:

- This is expected to apply on top of -next which contains fixes for the
  position of _etext on both arm and arm64.

- I couldn't detect a measurable performance change with these features
  enabled. Kernel build times were unchanged, hackbench was unchanged,
  etc. I think we could flip this to "on by default" at some point.

- The SLOB support extracted from grsecurity seems entirely broken. I
  have no idea what's going on there, I spent my time testing SLAB and
  SLUB. Having someone else look at SLOB would be nice, but this series
  doesn't depend on it.

Additional features that would be nice, but aren't blocking this series:

- Needs more architecture support for stack frame checking (only x86 now).


Thanks!

-Kees

[1] https://grsecurity.net/download.php "grsecurity - test kernel patch"
[2] http://www.openwall.com/lists/kernel-hardening/2016/05/19/5


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev