* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 9:13 ` [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory Mathias Krause
@ 2015-11-25 10:06 ` Clemens Ladisch
2015-11-25 11:14 ` PaX Team
2015-11-26 15:23 ` Daniel Micay
2015-11-25 11:05 ` PaX Team
` (2 subsequent siblings)
3 siblings, 2 replies; 41+ messages in thread
From: Clemens Ladisch @ 2015-11-25 10:06 UTC (permalink / raw)
To: Mathias Krause, kernel-hardening
Cc: linux-kernel, Kees Cook, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, x86-ml, Arnd Bergmann,
Michael Ellerman, linux-arch, PaX Team, Emese Revfy
Mathias Krause wrote:
> [...]
> So, prior extending the usage of the __read_only annotation some
> toolchain support is needed. Maybe a gcc plugin that'll warn/error on
> code that writes to such a variable but is not __init itself.
Or mark them as "const". This would require the initialization code to
cast it away, probably with a helper macro.
Regards,
Clemens
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 10:06 ` Clemens Ladisch
@ 2015-11-25 11:14 ` PaX Team
2015-11-26 15:23 ` Daniel Micay
1 sibling, 0 replies; 41+ messages in thread
From: PaX Team @ 2015-11-25 11:14 UTC (permalink / raw)
To: Mathias Krause, kernel-hardening, Clemens Ladisch
Cc: linux-kernel, Kees Cook, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, x86-ml, Arnd Bergmann,
Michael Ellerman, linux-arch, Emese Revfy
On 25 Nov 2015 at 11:06, Clemens Ladisch wrote:
> Mathias Krause wrote:
> > [...]
> > So, prior extending the usage of the __read_only annotation some
> > toolchain support is needed. Maybe a gcc plugin that'll warn/error on
> > code that writes to such a variable but is not __init itself.
>
> Or mark them as "const". This would require the initialization code to
> cast it away, probably with a helper macro.
no, that'd be undefined behaviour and in practice gcc would take advantage
of it and produce unintended (and quite broken) code. if the constified object
is modified from a different translation unit then the compiler is free
to assume that it can constant propagate its initialization value into uses,
completely breaking the code that (forcibly) writes to it.
however as a poor man's detector for such __read_only violations it's
possible to just make the object const temporarily (without casting away
the write attempts!), recompile the tree and see if any writes outside
__init functions pop up.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 10:06 ` Clemens Ladisch
2015-11-25 11:14 ` PaX Team
@ 2015-11-26 15:23 ` Daniel Micay
1 sibling, 0 replies; 41+ messages in thread
From: Daniel Micay @ 2015-11-26 15:23 UTC (permalink / raw)
To: kernel-hardening
[-- Attachment #1: Type: text/plain, Size: 749 bytes --]
On 25/11/15 05:06 AM, Clemens Ladisch wrote:
> Mathias Krause wrote:
>> [...]
>> So, prior extending the usage of the __read_only annotation some
>> toolchain support is needed. Maybe a gcc plugin that'll warn/error on
>> code that writes to such a variable but is not __init itself.
>
> Or mark them as "const". This would require the initialization code to
> cast it away, probably with a helper macro.
>
>
> Regards,
> Clemens
>
That would be undefined. It would only be safe if you exposed a const
pointer as the only way of accessing a non-const variable. It would be
silly to mangle the code simply to get a warning as it should be easy
enough to add to GCC. The usefulness isn't just limited to the kernel
either.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 9:13 ` [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory Mathias Krause
2015-11-25 10:06 ` Clemens Ladisch
@ 2015-11-25 11:05 ` PaX Team
2015-11-26 8:54 ` Ingo Molnar
2015-11-25 17:26 ` Kees Cook
2015-11-25 17:31 ` H. Peter Anvin
3 siblings, 1 reply; 41+ messages in thread
From: PaX Team @ 2015-11-25 11:05 UTC (permalink / raw)
To: kernel-hardening, Mathias Krause
Cc: linux-kernel, Kees Cook, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, x86-ml, Arnd Bergmann,
Michael Ellerman, linux-arch, Emese Revfy
On 25 Nov 2015 at 10:13, Mathias Krause wrote:
> I myself had some educating experience seeing my machine triple fault
> when resuming from a S3 sleep. The root cause was a variable that was
> annotated __read_only but that was (unnecessarily) modified during CPU
> bring-up phase. Debugging that kind of problems is sort of a PITA, you
> could imagine.
actually the kernel could silently recover from this given how the
page fault handler could easily determine that the fault address fell
into the data..read_only section and just silently undo the read-only
property, log the event to dmesg and retry the faulting access.
> So, prior extending the usage of the __read_only annotation some
> toolchain support is needed. Maybe a gcc plugin that'll warn/error on
> code that writes to such a variable but is not __init itself.
this is exactly what i suggested earlier in the constify thread ;).
note that this will produce false positives because __init* annotations
are not propagated everywhere they could be.
> The initify and checker plugins from the PaX patch might be worth to
> look at for that purpose, as they're doing similar things already.
one of our plans for initify is to add the discovery and propagation
of _init* annotations as well, it'd not only fix the false positives
mentioned above but also help reduce the kernel size (code/data/rodata).
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 11:05 ` PaX Team
@ 2015-11-26 8:54 ` Ingo Molnar
2015-11-26 9:57 ` PaX Team
2015-11-26 16:11 ` Andy Lutomirski
0 siblings, 2 replies; 41+ messages in thread
From: Ingo Molnar @ 2015-11-26 8:54 UTC (permalink / raw)
To: PaX Team
Cc: kernel-hardening, Mathias Krause, linux-kernel, Kees Cook,
Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
* PaX Team <pageexec@freemail.hu> wrote:
> On 25 Nov 2015 at 10:13, Mathias Krause wrote:
>
> > I myself had some educating experience seeing my machine triple fault
> > when resuming from a S3 sleep. The root cause was a variable that was
> > annotated __read_only but that was (unnecessarily) modified during CPU
> > bring-up phase. Debugging that kind of problems is sort of a PITA, you
> > could imagine.
( Sidenote: I don't think a ro-faults typically result in triple faults, but yeah,
even having a regular oops (followed by a hang or reboot) during such an
undebuggable state of the system is a major PITA. )
> actually the kernel could silently recover from this given how the page fault
> handler could easily determine that the fault address fell into the
> data..read_only section and just silently undo the read-only property, log the
> event to dmesg and retry the faulting access.
So a safer method would be to decode the faulting instruction, to skip it by
fixing up the return RIP and to log the event. It would be mostly equivalent to
trying to write to ROM (which get ignored as well), so it's a recoverable (and
debuggable) event.
We have all the necessary code in place in the kprobes code, see
arch/x86/lib/insn.c, it's a simplified x86 decoder that knows about instruction
length (but not about semantics).
Simple skipping plus setting arithmetic flags to init value should be enough I
think: I don't think we use fancy instructions to write to ro variables, such as
PUSH/POP with other side effects. If such instructions exist we could minimally
extend the decoder to do those fixups as well - in addition to double checking
that we skip simple instructions only with no side effects.
Can you see any fragility in such a technique?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-26 8:54 ` Ingo Molnar
@ 2015-11-26 9:57 ` PaX Team
2015-11-26 10:42 ` Ingo Molnar
2015-11-26 16:11 ` Andy Lutomirski
1 sibling, 1 reply; 41+ messages in thread
From: PaX Team @ 2015-11-26 9:57 UTC (permalink / raw)
To: Ingo Molnar
Cc: kernel-hardening, Mathias Krause, linux-kernel, Kees Cook,
Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
On 26 Nov 2015 at 9:54, Ingo Molnar wrote:
> * PaX Team <pageexec@freemail.hu> wrote:
>
> > actually the kernel could silently recover from this given how the page fault
> > handler could easily determine that the fault address fell into the
> > data..read_only section and just silently undo the read-only property, log the
> > event to dmesg and retry the faulting access.
>
> So a safer method would be to decode the faulting instruction, to skip it by
> fixing up the return RIP and to log the event. It would be mostly equivalent to
> trying to write to ROM (which get ignored as well), so it's a recoverable (and
> debuggable) event.
if by skipping you mean ignoring the write attempt then it's not a good idea
as it has a good chance to cause unexpected behaviour down the line.
e.g., imagine that the write was to a function pointer (even an entire ops
structure) or a boolean that controls some important feature for after-init
code. ignoring/dropping such writes could cause all kinds of logic bugs (if
not worse).
my somewhat related war story is that i once tried to constify machine_ops
(both the struct and the variable of the same name) directly and just forced
the writes in kvm/xen/etc via type casts. now i knew it was all undefined
behaviour but i didn't expect gcc to take advantage of it but it did (const
propagated the *initial* fptr values into the indirect calls by turning them
into direct calls) and which in turn prevented proper reboots for guests
(an event which obviously happens much later after init/boot to the great
puzzlement of end users and myself).
misusing __read_only and ignoring write attempts would effectively produce
the same misbehaviour as above so i strongly advise against it.
now i understand that the motivation is probably to preserve the read-only
property despite wrong use of __read_only but for this i'd suggest to simply
not make the silent recovery the default behaviour and enable it on the
kernel command line only. after all, this is expected to be a reproducible
problem and affected users can just reboot (in fact, they'd be advised to)
when it happens and get a proper report that they could then send back to lkml.
i also don't expect this to be a frequent problem as __read_only will have
to be handled carefully in every case and not just at the time of adding it
to a variable but also later during code evolution. as i suggested in the
constify plugin thread earlier, use of __read_only should be tied to compile
time checking of all uses of the affected variable to eliminate the entire
problem of problematic writes.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-26 9:57 ` PaX Team
@ 2015-11-26 10:42 ` Ingo Molnar
2015-11-26 12:14 ` PaX Team
0 siblings, 1 reply; 41+ messages in thread
From: Ingo Molnar @ 2015-11-26 10:42 UTC (permalink / raw)
To: PaX Team
Cc: kernel-hardening, Mathias Krause, linux-kernel, Kees Cook,
Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
* PaX Team <pageexec@freemail.hu> wrote:
> On 26 Nov 2015 at 9:54, Ingo Molnar wrote:
>
> > * PaX Team <pageexec@freemail.hu> wrote:
> >
> > > actually the kernel could silently recover from this given how the page fault
> > > handler could easily determine that the fault address fell into the
> > > data..read_only section and just silently undo the read-only property, log the
> > > event to dmesg and retry the faulting access.
> >
> > So a safer method would be to decode the faulting instruction, to skip it by
> > fixing up the return RIP and to log the event. It would be mostly equivalent
> > to trying to write to ROM (which get ignored as well), so it's a recoverable
> > (and debuggable) event.
>
> if by skipping you mean ignoring the write attempt then it's not a good idea as
> it has a good chance to cause unexpected behaviour down the line.
>
> e.g., imagine that the write was to a function pointer (even an entire ops
> structure) or a boolean that controls some important feature for after-init
> code. ignoring/dropping such writes could cause all kinds of logic bugs (if not
> worse).
Well, the typical case is that it's a logic bug to _do_ the write: the structure
was marked readonly for a reason but some init code re-runs during suspend or so.
But yes, logic bugs might trigger - but that is true in the opposite case as well,
if we do the write despite it being marked readonly:
> my somewhat related war story is that i once tried to constify machine_ops (both
> the struct and the variable of the same name) directly and just forced the
> writes in kvm/xen/etc via type casts. now i knew it was all undefined behaviour
> but i didn't expect gcc to take advantage of it but it did (const propagated the
> *initial* fptr values into the indirect calls by turning them into direct calls)
> and which in turn prevented proper reboots for guests (an event which obviously
> happens much later after init/boot to the great puzzlement of end users and
> myself).
>
> misusing __read_only and ignoring write attempts would effectively produce the
> same misbehaviour as above so i strongly advise against it.
No, the difference to the GCC related aliasing bug is that with my technique the
kernel would immediately produce a very visible kernel warning, which is a very
clear sign that is wrong - and with a very clear backtrace in the warning that
points right to the problematic code - which signature shows us (and users) what
is wrong.
So your example is not comparable at all.
Plus the truly paranoid might panic/halt the system on such warnings, so for
highly secure systems there's a way to not even allow the possibility of logic
bugs. (at the cost of stopping the system when a bug triggers.)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-26 10:42 ` Ingo Molnar
@ 2015-11-26 12:14 ` PaX Team
2015-11-27 8:05 ` Ingo Molnar
0 siblings, 1 reply; 41+ messages in thread
From: PaX Team @ 2015-11-26 12:14 UTC (permalink / raw)
To: Ingo Molnar
Cc: kernel-hardening, Mathias Krause, linux-kernel, Kees Cook,
Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
On 26 Nov 2015 at 11:42, Ingo Molnar wrote:
> * PaX Team <pageexec@freemail.hu> wrote:
>
> > On 26 Nov 2015 at 9:54, Ingo Molnar wrote:
>
> > e.g., imagine that the write was to a function pointer (even an entire ops
> > structure) or a boolean that controls some important feature for after-init
> > code. ignoring/dropping such writes could cause all kinds of logic bugs (if not
> > worse).
>
> Well, the typical case is that it's a logic bug to _do_ the write: the structure
> was marked readonly for a reason but some init code re-runs during suspend or so.
that's actually not the typical case in my experience, but rather these two:
1. initial mistake: someone didn't actually check whether the given object can
be __read_only
2. code evolution: an object that was once written by __init code only (and
thus proactively subjected to __read_only) gets modified by non-init code
due to later changes
what you described above is a third case where there's a latent bug to begin
(unintended write) with that __read_only merely exposes but doesn't create
itself, unlike the two cases above (intended writes getting caught by wrong
use of __read_only).
> But yes, logic bugs might trigger - but that is true in the opposite case as well,
> if we do the write despite it being marked readonly:
not really, the two cases above are not a priori bugs, they become bugs due
to using __read_only without due care (which is why i suggested to detect
them at compile time).
> > misusing __read_only and ignoring write attempts would effectively produce the
> > same misbehaviour as above so i strongly advise against it.
>
> No, the difference to the GCC related aliasing bug is that with my technique the
> kernel would immediately produce a very visible kernel warning, which is a very
> clear sign that is wrong - and with a very clear backtrace in the warning that
> points right to the problematic code - which signature shows us (and users) what
> is wrong.
my proposal would produce the exact same reports, the difference is in letting
the write attempt succeed vs. skipping it. this latter step is what is wrong
since it introduces at least a logic bug the same way the constprop optimization
created a logic bug.
> Plus the truly paranoid might panic/halt the system on such warnings, so for
> highly secure systems there's a way to not even allow the possibility of logic
> bugs. (at the cost of stopping the system when a bug triggers.)
this would/should be the default behaviour, i.e., no attempt at being smart by
either allowing or skipping the faulting insn, just report the event and trigger
whatever fancy future exploit reaction mechanism will get into the kernel.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-26 12:14 ` PaX Team
@ 2015-11-27 8:05 ` Ingo Molnar
2015-11-27 15:29 ` PaX Team
0 siblings, 1 reply; 41+ messages in thread
From: Ingo Molnar @ 2015-11-27 8:05 UTC (permalink / raw)
To: PaX Team, Linus Torvalds
Cc: kernel-hardening, Mathias Krause, linux-kernel, Kees Cook,
Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
* PaX Team <pageexec@freemail.hu> wrote:
> On 26 Nov 2015 at 11:42, Ingo Molnar wrote:
>
> > * PaX Team <pageexec@freemail.hu> wrote:
> >
> > > On 26 Nov 2015 at 9:54, Ingo Molnar wrote:
> >
> > > e.g., imagine that the write was to a function pointer (even an entire ops
> > > structure) or a boolean that controls some important feature for after-init
> > > code. ignoring/dropping such writes could cause all kinds of logic bugs (if not
> > > worse).
> >
> > Well, the typical case is that it's a logic bug to _do_ the write: the structure
> > was marked readonly for a reason but some init code re-runs during suspend or so.
>
> that's actually not the typical case in my experience, but rather these two:
>
> 1. initial mistake: someone didn't actually check whether the given object can
> be __read_only
>
> 2. code evolution: an object that was once written by __init code only (and
> thus proactively subjected to __read_only) gets modified by non-init code
> due to later changes
>
> what you described above is a third case where there's a latent bug to begin
> (unintended write) with that __read_only merely exposes but doesn't create
> itself, unlike the two cases above (intended writes getting caught by wrong use
> of __read_only).
You are right, I concede this part of the argument - what you describe is probably
the most typical way to get ro-faults.
I do maintain the (sub-)argument that oopsing or relying on tooling help years
down the line is vastly inferior to fixing up the problem and generating a
one-time stack dump so that kernel developers have a chance to fix the bug. The
sooner we detect and dump such information the more likely it is that such bugs
don't get into end user kernel versions.
> my proposal would produce the exact same reports, the difference is in letting
> the write attempt succeed vs. skipping it. this latter step is what is wrong
> since it introduces at least a logic bug the same way the constprop optimization
> created a logic bug.
Yes, you are right and I agree.
Does anyone want to submit such a patch for upstream? Looks like a good change.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 8:05 ` Ingo Molnar
@ 2015-11-27 15:29 ` PaX Team
2015-11-27 16:30 ` Andy Lutomirski
2015-11-29 8:08 ` Ingo Molnar
0 siblings, 2 replies; 41+ messages in thread
From: PaX Team @ 2015-11-27 15:29 UTC (permalink / raw)
To: Linus Torvalds, Ingo Molnar
Cc: kernel-hardening, Mathias Krause, linux-kernel, Kees Cook,
Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
On 27 Nov 2015 at 9:05, Ingo Molnar wrote:
> * PaX Team <pageexec@freemail.hu> wrote:
>
> > On 26 Nov 2015 at 11:42, Ingo Molnar wrote:
> >
> > > * PaX Team <pageexec@freemail.hu> wrote:
> > that's actually not the typical case in my experience, but rather these two:
> >
> > 1. initial mistake: someone didn't actually check whether the given object can
> > be __read_only
> >
> > 2. code evolution: an object that was once written by __init code only (and
> > thus proactively subjected to __read_only) gets modified by non-init code
> > due to later changes
> >
> > what you described above is a third case where there's a latent bug to begin
> > (unintended write) with that __read_only merely exposes but doesn't create
> > itself, unlike the two cases above (intended writes getting caught by wrong use
> > of __read_only).
>
> You are right, I concede this part of the argument - what you describe is probably
> the most typical way to get ro-faults.
>
> I do maintain the (sub-)argument that oopsing or relying on tooling help years
> down the line is vastly inferior to fixing up the problem and generating a
> one-time stack dump so that kernel developers have a chance to fix the bug. The
> sooner we detect and dump such information the more likely it is that such bugs
> don't get into end user kernel versions.
i don't see the compile time vs. runtime detection as 'competing' approaches,
both have their own role. in general, i think it's safe to say that compile
time problem detection is preferred to the runtime one since it subjects less
users to the side effects of the bug. runtime detection is needed to augment
(even complete) the coverage that compile time detection may not be able to
provide.
that said, for __read_only related problems the compiler can actually do a
pretty good job, basically it could detect most of them except special cases
where the 'bad' write is somehow hidden from it. the only examples i recall
are like the one that Mathias already mentioned where the 'bad' write was
done from asm code or out-of-kernel code (think UEFI runtime services) that
is obviously not visible to the compiler (the resume/mmu_cr4_features problem
also happens to be an example where runtime detection did not help due to the
circumstances).
so let me summarize how i expect the runtime detection part to work:
1. in normal use any write attempt to read-only kernel data should only
be reported as usual (the oops info already has rip/cr2/backtrace),
but no smart recovery attempts should be made since they may end up
actually helping a real exploit attempt.
2. if necessary for debugging purposes (i.e., when the above reporting
mechanism didn't produce the necessary logs and the problem is
reproducible and wasn't an attack), a kernel command line option can
be used to make an attempt at smart recovery instead of oopsing (but
the same information would still be reported of course).
for this smart recovery we differ(ed?) in opinion, i say that allowing
the write in this case (vs. ignoring it) is the least likely to introduce
a logic bug (and its cascading effects) since the expected problem is
to be case #1 or #2 above (i.e., the write is intended but prevented
by __read_only).
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 15:29 ` PaX Team
@ 2015-11-27 16:30 ` Andy Lutomirski
2015-11-29 8:08 ` Ingo Molnar
1 sibling, 0 replies; 41+ messages in thread
From: Andy Lutomirski @ 2015-11-27 16:30 UTC (permalink / raw)
To: PaX Team
Cc: Linus Torvalds, Ingo Molnar, kernel-hardening, Mathias Krause,
linux-kernel, Kees Cook, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Fri, Nov 27, 2015 at 7:29 AM, PaX Team <pageexec@freemail.hu> wrote:
> On 27 Nov 2015 at 9:05, Ingo Molnar wrote:
>
>> * PaX Team <pageexec@freemail.hu> wrote:
>>
>> > On 26 Nov 2015 at 11:42, Ingo Molnar wrote:
>> >
>> > > * PaX Team <pageexec@freemail.hu> wrote:
>> > that's actually not the typical case in my experience, but rather these two:
>> >
>> > 1. initial mistake: someone didn't actually check whether the given object can
>> > be __read_only
>> >
>> > 2. code evolution: an object that was once written by __init code only (and
>> > thus proactively subjected to __read_only) gets modified by non-init code
>> > due to later changes
>> >
>> > what you described above is a third case where there's a latent bug to begin
>> > (unintended write) with that __read_only merely exposes but doesn't create
>> > itself, unlike the two cases above (intended writes getting caught by wrong use
>> > of __read_only).
>>
>> You are right, I concede this part of the argument - what you describe is probably
>> the most typical way to get ro-faults.
>>
>> I do maintain the (sub-)argument that oopsing or relying on tooling help years
>> down the line is vastly inferior to fixing up the problem and generating a
>> one-time stack dump so that kernel developers have a chance to fix the bug. The
>> sooner we detect and dump such information the more likely it is that such bugs
>> don't get into end user kernel versions.
>
> i don't see the compile time vs. runtime detection as 'competing' approaches,
> both have their own role. in general, i think it's safe to say that compile
> time problem detection is preferred to the runtime one since it subjects less
> users to the side effects of the bug. runtime detection is needed to augment
> (even complete) the coverage that compile time detection may not be able to
> provide.
>
> that said, for __read_only related problems the compiler can actually do a
> pretty good job, basically it could detect most of them except special cases
> where the 'bad' write is somehow hidden from it. the only examples i recall
> are like the one that Mathias already mentioned where the 'bad' write was
> done from asm code or out-of-kernel code (think UEFI runtime services) that
> is obviously not visible to the compiler (the resume/mmu_cr4_features problem
> also happens to be an example where runtime detection did not help due to the
> circumstances).
>
> so let me summarize how i expect the runtime detection part to work:
>
> 1. in normal use any write attempt to read-only kernel data should only
> be reported as usual (the oops info already has rip/cr2/backtrace),
> but no smart recovery attempts should be made since they may end up
> actually helping a real exploit attempt.
>
> 2. if necessary for debugging purposes (i.e., when the above reporting
> mechanism didn't produce the necessary logs and the problem is
> reproducible and wasn't an attack), a kernel command line option can
> be used to make an attempt at smart recovery instead of oopsing (but
> the same information would still be reported of course).
>
> for this smart recovery we differ(ed?) in opinion, i say that allowing
> the write in this case (vs. ignoring it) is the least likely to introduce
> a logic bug (and its cascading effects) since the expected problem is
> to be case #1 or #2 above (i.e., the write is intended but prevented
> by __read_only).
>
So maybe we should think about doing this recovery as part of oops
processing. That is, we oops as usual, but rather than killing the
task or spinning, we allow the post-oops code to try to recover (if
enabled). That recovery step decodes the instruction and takes some
action. In this example, if it's a write to ro-after-init memory,
then maybe we un-write-protect it and resume.
I agree with Linus' old sentiment as least insofar as trying to decode
things pre-OOPS is a bad idea: we don't want that decoding to
interfere with our primary goal, which is printing the OOPS.
I would argue that, if this is okay, we do exactly the same thing for
failed msr access. We still oops, but we try to march on.
We could consider a default setting in which we "recover" from oops
until init starts and then we revert to old behavior (oopses kill the
task).
--Andy
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 15:29 ` PaX Team
2015-11-27 16:30 ` Andy Lutomirski
@ 2015-11-29 8:08 ` Ingo Molnar
2015-11-29 11:15 ` PaX Team
1 sibling, 1 reply; 41+ messages in thread
From: Ingo Molnar @ 2015-11-29 8:08 UTC (permalink / raw)
To: PaX Team
Cc: Linus Torvalds, kernel-hardening, Mathias Krause, linux-kernel,
Kees Cook, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
* PaX Team <pageexec@freemail.hu> wrote:
> i don't see the compile time vs. runtime detection as 'competing' approaches,
> both have their own role. [...]
That's true - but only as long as 'this can be solved in tooling!' is not used as
an excuse to oppose the runtime solution and we end up doing neither.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-29 8:08 ` Ingo Molnar
@ 2015-11-29 11:15 ` PaX Team
2015-11-29 15:39 ` Ingo Molnar
0 siblings, 1 reply; 41+ messages in thread
From: PaX Team @ 2015-11-29 11:15 UTC (permalink / raw)
To: Ingo Molnar
Cc: Linus Torvalds, kernel-hardening, Mathias Krause, linux-kernel,
Kees Cook, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On 29 Nov 2015 at 9:08, Ingo Molnar wrote:
>
> * PaX Team <pageexec@freemail.hu> wrote:
>
> > i don't see the compile time vs. runtime detection as 'competing' approaches,
> > both have their own role. [...]
>
> That's true - but only as long as 'this can be solved in tooling!' is not used as
> an excuse to oppose the runtime solution and we end up doing neither.
actually, i already voiced my opinion elsewhere in the constify thread on
the kernel hardening list that adding/using __read_only is somewhat premature
without also adding the compile time verification part (as part of the
constify plugin for example). right now its use on the embedded vdso image
is simple and easy to verify but once people begin to add it to variables
that the compiler knows and cares about (say, the ops structures) then
things can become fragile without compile checking. so yes, i'd also advise
to get such tooling in *before* more __read_only usage is added.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-29 11:15 ` PaX Team
@ 2015-11-29 15:39 ` Ingo Molnar
2015-11-29 18:05 ` Mathias Krause
0 siblings, 1 reply; 41+ messages in thread
From: Ingo Molnar @ 2015-11-29 15:39 UTC (permalink / raw)
To: PaX Team
Cc: Linus Torvalds, kernel-hardening, Mathias Krause, linux-kernel,
Kees Cook, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
* PaX Team <pageexec@freemail.hu> wrote:
> On 29 Nov 2015 at 9:08, Ingo Molnar wrote:
>
> >
> > * PaX Team <pageexec@freemail.hu> wrote:
> >
> > > i don't see the compile time vs. runtime detection as 'competing' approaches,
> > > both have their own role. [...]
> >
> > That's true - but only as long as 'this can be solved in tooling!' is not used as
> > an excuse to oppose the runtime solution and we end up doing neither.
>
> actually, i already voiced my opinion elsewhere in the constify thread on the
> kernel hardening list that adding/using __read_only is somewhat premature
> without also adding the compile time verification part (as part of the constify
> plugin for example). right now its use on the embedded vdso image is simple and
> easy to verify but once people begin to add it to variables that the compiler
> knows and cares about (say, the ops structures) then things can become fragile
> without compile checking. so yes, i'd also advise to get such tooling in
> *before* more __read_only usage is added.
I think you are mistaken there: if we add the page fault fixup to make sure we
don't crash if a read-only variable is accessed, then we'll have most of the
benefits of read-only mappings and no fragility - without having to wait for
tooling.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-29 15:39 ` Ingo Molnar
@ 2015-11-29 18:05 ` Mathias Krause
2015-11-30 8:01 ` Ingo Molnar
0 siblings, 1 reply; 41+ messages in thread
From: Mathias Krause @ 2015-11-29 18:05 UTC (permalink / raw)
To: Ingo Molnar
Cc: PaX Team, Linus Torvalds, kernel-hardening, linux-kernel,
Kees Cook, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On 29 November 2015 at 16:39, Ingo Molnar <mingo@kernel.org> wrote:
>
> * PaX Team <pageexec@freemail.hu> wrote:
>
>> On 29 Nov 2015 at 9:08, Ingo Molnar wrote:
>>
>> >
>> > * PaX Team <pageexec@freemail.hu> wrote:
>> >
>> > > i don't see the compile time vs. runtime detection as 'competing' approaches,
>> > > both have their own role. [...]
>> >
>> > That's true - but only as long as 'this can be solved in tooling!' is not used as
>> > an excuse to oppose the runtime solution and we end up doing neither.
>>
>> actually, i already voiced my opinion elsewhere in the constify thread on the
>> kernel hardening list that adding/using __read_only is somewhat premature
>> without also adding the compile time verification part (as part of the constify
>> plugin for example). right now its use on the embedded vdso image is simple and
>> easy to verify but once people begin to add it to variables that the compiler
>> knows and cares about (say, the ops structures) then things can become fragile
>> without compile checking. so yes, i'd also advise to get such tooling in
>> *before* more __read_only usage is added.
>
> I think you are mistaken there: if we add the page fault fixup to make sure we
> don't crash if a read-only variable is accessed, then we'll have most of the
> benefits of read-only mappings and no fragility - without having to wait for
> tooling.
I guess the point PaX Team (and me earlier in this thread) wanted to
make is that having misuse detection *only* at run-time will make
those kind of bugs visible too late -- as late as on the wrong write
attempt actually happening. It would be much better to have the
compiler complain about invalid write statements already during
compilation, much like it does when one wants to assign some value to
a const object.
Having the page fault handler being able to recover from
__ro_after_init faults is a nice feature to support users, actually
being able to report bugs. But it shouldn't be the only way to detect
those kinds of bugs. In fact, we've tools in our toolchain that try to
detect and flag wrong usage of __init / __exit, so why not cover
__ro_after_init as well? Admitted, that won't be possible with modpost
in its current state, but would require a compiler extension instead.
Its current non-existence shouldn't be a show-stopper for
__ro_after_init but the very next step to take care of before
extending the usage of that annotation.
Just my 2ct,
Mathias
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-29 18:05 ` Mathias Krause
@ 2015-11-30 8:01 ` Ingo Molnar
0 siblings, 0 replies; 41+ messages in thread
From: Ingo Molnar @ 2015-11-30 8:01 UTC (permalink / raw)
To: Mathias Krause
Cc: PaX Team, Linus Torvalds, kernel-hardening, linux-kernel,
Kees Cook, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
* Mathias Krause <minipli@googlemail.com> wrote:
> On 29 November 2015 at 16:39, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * PaX Team <pageexec@freemail.hu> wrote:
> >
> >> On 29 Nov 2015 at 9:08, Ingo Molnar wrote:
> >>
> >> >
> >> > * PaX Team <pageexec@freemail.hu> wrote:
> >> >
> >> > > i don't see the compile time vs. runtime detection as 'competing' approaches,
> >> > > both have their own role. [...]
> >> >
> >> > That's true - but only as long as 'this can be solved in tooling!' is not used as
> >> > an excuse to oppose the runtime solution and we end up doing neither.
> >>
> >> actually, i already voiced my opinion elsewhere in the constify thread on the
> >> kernel hardening list that adding/using __read_only is somewhat premature
> >> without also adding the compile time verification part (as part of the constify
> >> plugin for example). right now its use on the embedded vdso image is simple and
> >> easy to verify but once people begin to add it to variables that the compiler
> >> knows and cares about (say, the ops structures) then things can become fragile
> >> without compile checking. so yes, i'd also advise to get such tooling in
> >> *before* more __read_only usage is added.
> >
> > I think you are mistaken there: if we add the page fault fixup to make sure we
> > don't crash if a read-only variable is accessed, then we'll have most of the
> > benefits of read-only mappings and no fragility - without having to wait for
> > tooling.
>
> I guess the point PaX Team (and me earlier in this thread) wanted to
> make is that having misuse detection *only* at run-time will make
> those kind of bugs visible too late -- as late as on the wrong write
> attempt actually happening. It would be much better to have the
> compiler complain about invalid write statements already during
> compilation, much like it does when one wants to assign some value to
> a const object.
Well, the runtime warning comes "later", that does not automatically transform
into "too late".
These things are relatively rare. Whether the warning is in tooling or runtime (or
both) is relatively immaterial in that regard: having it in tooling would be nice,
but it's not fool-proof either: say if tooling leaves an easy backdoor like
allowing a type cast to supress warnings then it turns into a hard to debug
problem again.
So I think we need both, starting with the runtime warning which is easy to
implement and can be merged right away.
But reality is that upstream tooling changes, especially ones involving compiler
changes move at a glacier's pace in comparison. Based on past experience I am also
pretty pessimistic: I'm 90% certain that this 'tooling feature' won't be
implemented in the next 10 years, so I'd like to concentrate on what we can do
here and now: the runtime warning and recovery without crashing.
If anyone sends patches for that I'll apply them.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-26 8:54 ` Ingo Molnar
2015-11-26 9:57 ` PaX Team
@ 2015-11-26 16:11 ` Andy Lutomirski
2015-11-27 7:59 ` Ingo Molnar
1 sibling, 1 reply; 41+ messages in thread
From: Andy Lutomirski @ 2015-11-26 16:11 UTC (permalink / raw)
To: Ingo Molnar
Cc: PaX Team, kernel-hardening, Mathias Krause, linux-kernel,
Kees Cook, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, x86-ml,
Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
On Thu, Nov 26, 2015 at 12:54 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * PaX Team <pageexec@freemail.hu> wrote:
>
>> On 25 Nov 2015 at 10:13, Mathias Krause wrote:
>>
>> > I myself had some educating experience seeing my machine triple fault
>> > when resuming from a S3 sleep. The root cause was a variable that was
>> > annotated __read_only but that was (unnecessarily) modified during CPU
>> > bring-up phase. Debugging that kind of problems is sort of a PITA, you
>> > could imagine.
>
> ( Sidenote: I don't think a ro-faults typically result in triple faults, but yeah,
> even having a regular oops (followed by a hang or reboot) during such an
> undebuggable state of the system is a major PITA. )
>
>> actually the kernel could silently recover from this given how the page fault
>> handler could easily determine that the fault address fell into the
>> data..read_only section and just silently undo the read-only property, log the
>> event to dmesg and retry the faulting access.
>
> So a safer method would be to decode the faulting instruction, to skip it by
> fixing up the return RIP and to log the event. It would be mostly equivalent to
> trying to write to ROM (which get ignored as well), so it's a recoverable (and
> debuggable) event.
>
> We have all the necessary code in place in the kprobes code, see
> arch/x86/lib/insn.c, it's a simplified x86 decoder that knows about instruction
> length (but not about semantics).
>
> Simple skipping plus setting arithmetic flags to init value should be enough I
> think: I don't think we use fancy instructions to write to ro variables, such as
> PUSH/POP with other side effects. If such instructions exist we could minimally
> extend the decoder to do those fixups as well - in addition to double checking
> that we skip simple instructions only with no side effects.
>
> Can you see any fragility in such a technique?
>
After Linus shot down my rdmsr/rwmsr decoding patch, good luck...
More seriously, though, I think this is mostly just like any other
in-kernel fault. We failed, me might be under attack, let's oops. In
the particular case of suspend/resume, we could consider a debug flag
to allow writes to these variables during suspend/resume. In fact,
that might even be a reasonable default. We might want to allow
writes during module unload as well.
For everything else, we should probably focus more on getting OOPSes
to display reliably, which is supposed to work but, on my shiny new
i915-based laptop, is clearly not ready yet (I oopsed it yesterday due
to my own bug and all I had to show for it was a blinking capslock
key, and yes, modesetting works).
--Andy
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-26 16:11 ` Andy Lutomirski
@ 2015-11-27 7:59 ` Ingo Molnar
2015-11-27 18:00 ` Linus Torvalds
0 siblings, 1 reply; 41+ messages in thread
From: Ingo Molnar @ 2015-11-27 7:59 UTC (permalink / raw)
To: Andy Lutomirski, Linus Torvalds
Cc: PaX Team, kernel-hardening, Mathias Krause, linux-kernel,
Kees Cook, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, x86-ml,
Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
* Andy Lutomirski <luto@amacapital.net> wrote:
> > Can you see any fragility in such a technique?
>
> After Linus shot down my rdmsr/rwmsr decoding patch, good luck...
I think that case was entirely different, but I've Cc:-ed Linus to shoot my idea
down if it's crap.
> More seriously, though, I think this is mostly just like any other in-kernel
> fault. We failed, me might be under attack, let's oops. In the particular case
> of suspend/resume, we could consider a debug flag to allow writes to these
> variables during suspend/resume. In fact, that might even be a reasonable
> default. We might want to allow writes during module unload as well.
We are getting the _same_ information: we generate a reliable stack trace right
there. We don't ignore anything.
What my suggestion would do is to turn a 'sure system crasher' into a
'informational debug message'.
On today's typical desktop systems I can tell you with 110% confidence that the
vast majority of 'system crasher' oopses never reaches a kernel developer's
attention, because the oops message is not propagated to the user, while the 'dump
stack trace and try to continue' approach will result in proper bugzillas.
And that's really an important distinction IMHO. Getting debug info out of the
system is very important - and those who are paranoid can set a Kconfig value to
crash their systems on any hint of a problem.
> For everything else, we should probably focus more on getting OOPSes to display
> reliably, which is supposed to work but, on my shiny new i915-based laptop, is
> clearly not ready yet (I oopsed it yesterday due to my own bug and all I had to
> show for it was a blinking capslock key, and yes, modesetting works).
That's absolutely true as well but an independent issue: it does not invalidate my
argument that is based on the status quo, which is that the vast majority
panics/oopses, _especially_ during suspend/resume that was mentioned in this case,
does not reach any kernel developer.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 7:59 ` Ingo Molnar
@ 2015-11-27 18:00 ` Linus Torvalds
2015-11-27 18:03 ` Linus Torvalds
2015-11-27 20:03 ` Kees Cook
0 siblings, 2 replies; 41+ messages in thread
From: Linus Torvalds @ 2015-11-27 18:00 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andy Lutomirski, PaX Team, kernel-hardening, Mathias Krause,
linux-kernel, Kees Cook, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Thu, Nov 26, 2015 at 11:59 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> > Can you see any fragility in such a technique?
>>
>> After Linus shot down my rdmsr/rwmsr decoding patch, good luck...
>
> I think that case was entirely different, but I've Cc:-ed Linus to shoot my idea
> down if it's crap.
Yeah, no, I hate it. I'm with the PaX team on this one - I think there
are three valid responses, and I think we might want to have a dynamic
config option (kernel command line or proc or whatever) to pick
between the two:
- just oops and kill the machine, like for any other unhandled kernel
page fault. This is probably what you should have on a server
- print a warning and a backtrace, and just mark the page read-write
so that the machine survives, but we get notified and can fix whatever
broken code
- have an option to disable the RO data logic.
I think that second option is good for debugging. In some places,
oopses that kill things are just too hard to debug (ie it might be the
modesetting or early boot or whatever).
In fact, I think we should _start_ with the second option - perhaps
just during the rc's - and then when we're pretty sure all the silly
bugs it finds (maybe none, who knows) are handled, we should go to the
first one.
The third option would be purely for "user that cannot fix things
directly and has reported the problem can now turn off the distracting
warning". We should never default to it.
Trying to actually *recover* any other way thanm by turning the area
read-write is just too damn fragile. You can't just skip over the
instruction that does the write - there are flags values etc that get
updated by read-modify-write instructions, but as PaX says, there nmay
also be subsequent logic that gets confused and actually introduces
even *more* problems downstream if the write is just discarded.
So maybe we could have some kind of "mark it read-only again later"
thing that tries to make sure it doesn't stay writable for a long
time, but quite frankly, I don't think it's worth it. Once the write
has been done, and the warning has been emitted, there's likely very
little upside to then trying to close the barn doors after that horse
has bolted.
Linus
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 18:00 ` Linus Torvalds
@ 2015-11-27 18:03 ` Linus Torvalds
2015-11-27 20:03 ` Kees Cook
1 sibling, 0 replies; 41+ messages in thread
From: Linus Torvalds @ 2015-11-27 18:03 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andy Lutomirski, PaX Team, kernel-hardening, Mathias Krause,
linux-kernel, Kees Cook, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Fri, Nov 27, 2015 at 10:00 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> - just oops and kill the machine, like for any other unhandled kernel
> page fault. This is probably what you should have on a server
Just to clarify: the "just oops" obviously doesn't have to kill the
machine, it depends on what your oops policy is, with the default
obviously being the normal "kill that particular thread" if at all
possible.
Machine-killing is appropriate in some secure situations, but most of
the time it just makes it too damn hard to debug since the error often
doesn't get logged. In some situations we obviously can't avoid it,
but..
Linus
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 18:00 ` Linus Torvalds
2015-11-27 18:03 ` Linus Torvalds
@ 2015-11-27 20:03 ` Kees Cook
2015-11-27 20:09 ` Andy Lutomirski
1 sibling, 1 reply; 41+ messages in thread
From: Kees Cook @ 2015-11-27 20:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Andy Lutomirski, PaX Team, kernel-hardening,
Mathias Krause, linux-kernel, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Fri, Nov 27, 2015 at 10:00 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Nov 26, 2015 at 11:59 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Andy Lutomirski <luto@amacapital.net> wrote:
>>
>>> > Can you see any fragility in such a technique?
>>>
>>> After Linus shot down my rdmsr/rwmsr decoding patch, good luck...
>>
>> I think that case was entirely different, but I've Cc:-ed Linus to shoot my idea
>> down if it's crap.
>
> Yeah, no, I hate it. I'm with the PaX team on this one - I think there
> are three valid responses, and I think we might want to have a dynamic
> config option (kernel command line or proc or whatever) to pick
> between the two:
>
> - just oops and kill the machine, like for any other unhandled kernel
> page fault. This is probably what you should have on a server
This is how the v2 series works now.
> - print a warning and a backtrace, and just mark the page read-write
> so that the machine survives, but we get notified and can fix whatever
> broken code
This seems very easy to add. Should I basically reverse the effects of
mark_rodata_ro(), or should I only make the new ro-after-init section
as RW? (I think the former would be easier.)
> - have an option to disable the RO data logic.
I added this as "rodata=off" in the v2 series.
> I think that second option is good for debugging. In some places,
> oopses that kill things are just too hard to debug (ie it might be the
> modesetting or early boot or whatever).
>
> In fact, I think we should _start_ with the second option - perhaps
> just during the rc's - and then when we're pretty sure all the silly
> bugs it finds (maybe none, who knows) are handled, we should go to the
> first one.
>
> The third option would be purely for "user that cannot fix things
> directly and has reported the problem can now turn off the distracting
> warning". We should never default to it.
>
> Trying to actually *recover* any other way thanm by turning the area
> read-write is just too damn fragile. You can't just skip over the
> instruction that does the write - there are flags values etc that get
> updated by read-modify-write instructions, but as PaX says, there nmay
> also be subsequent logic that gets confused and actually introduces
> even *more* problems downstream if the write is just discarded.
>
> So maybe we could have some kind of "mark it read-only again later"
> thing that tries to make sure it doesn't stay writable for a long
> time, but quite frankly, I don't think it's worth it. Once the write
> has been done, and the warning has been emitted, there's likely very
> little upside to then trying to close the barn doors after that horse
> has bolted.
>
> Linus
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 20:03 ` Kees Cook
@ 2015-11-27 20:09 ` Andy Lutomirski
2015-11-29 8:05 ` Ingo Molnar
0 siblings, 1 reply; 41+ messages in thread
From: Andy Lutomirski @ 2015-11-27 20:09 UTC (permalink / raw)
To: Kees Cook
Cc: Linus Torvalds, Ingo Molnar, PaX Team, kernel-hardening,
Mathias Krause, linux-kernel, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Fri, Nov 27, 2015 at 12:03 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Nov 27, 2015 at 10:00 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Nov 26, 2015 at 11:59 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>> * Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>>> > Can you see any fragility in such a technique?
>>>>
>>>> After Linus shot down my rdmsr/rwmsr decoding patch, good luck...
>>>
>>> I think that case was entirely different, but I've Cc:-ed Linus to shoot my idea
>>> down if it's crap.
>>
>> Yeah, no, I hate it. I'm with the PaX team on this one - I think there
>> are three valid responses, and I think we might want to have a dynamic
>> config option (kernel command line or proc or whatever) to pick
>> between the two:
>>
>> - just oops and kill the machine, like for any other unhandled kernel
>> page fault. This is probably what you should have on a server
>
> This is how the v2 series works now.
>
>> - print a warning and a backtrace, and just mark the page read-write
>> so that the machine survives, but we get notified and can fix whatever
>> broken code
>
> This seems very easy to add. Should I basically reverse the effects of
> mark_rodata_ro(), or should I only make the new ro-after-init section
> as RW? (I think the former would be easier.)
I'd suggest verifying that the page in question is
.data..ro_after_init and, if so, marking that one page RW.
--Andy
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-27 20:09 ` Andy Lutomirski
@ 2015-11-29 8:05 ` Ingo Molnar
2015-11-30 21:14 ` H. Peter Anvin
0 siblings, 1 reply; 41+ messages in thread
From: Ingo Molnar @ 2015-11-29 8:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, Linus Torvalds, PaX Team, kernel-hardening,
Mathias Krause, linux-kernel, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
* Andy Lutomirski <luto@amacapital.net> wrote:
> >> - print a warning and a backtrace, and just mark the page read-write
> >> so that the machine survives, but we get notified and can fix whatever
> >> broken code
> >
> > This seems very easy to add. Should I basically reverse the effects of
> > mark_rodata_ro(), or should I only make the new ro-after-init section as RW?
> > (I think the former would be easier.)
>
> I'd suggest verifying that the page in question is .data..ro_after_init and, if
> so, marking that one page RW.
Yes, this was PaX's suggestion as well, and I agree: doing that turns a quite
possibly unrecoverable boot/shutdown time or suspend/resume time (suspend is
really a special category of 'bootup') crasher oops into a more informative stack
dump.
These ro related faults tend to trigger when init/deinit is running, and oopsing
in those sequences is typically a lot less survivable than say oopsing in a high
level system call while not holding locks.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-29 8:05 ` Ingo Molnar
@ 2015-11-30 21:14 ` H. Peter Anvin
2015-11-30 21:33 ` Kees Cook
0 siblings, 1 reply; 41+ messages in thread
From: H. Peter Anvin @ 2015-11-30 21:14 UTC (permalink / raw)
To: Ingo Molnar, Andy Lutomirski
Cc: Kees Cook, Linus Torvalds, PaX Team, kernel-hardening,
Mathias Krause, linux-kernel, Ingo Molnar, Thomas Gleixner,
x86-ml, Arnd Bergmann, Michael Ellerman, linux-arch, Emese Revfy
On 11/29/15 00:05, Ingo Molnar wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>>>> - print a warning and a backtrace, and just mark the page read-write
>>>> so that the machine survives, but we get notified and can fix whatever
>>>> broken code
>>>
>>> This seems very easy to add. Should I basically reverse the effects of
>>> mark_rodata_ro(), or should I only make the new ro-after-init section as RW?
>>> (I think the former would be easier.)
>>
>> I'd suggest verifying that the page in question is .data..ro_after_init and, if
>> so, marking that one page RW.
>
> Yes, this was PaX's suggestion as well, and I agree: doing that turns a quite
> possibly unrecoverable boot/shutdown time or suspend/resume time (suspend is
> really a special category of 'bootup') crasher oops into a more informative stack
> dump.
>
> These ro related faults tend to trigger when init/deinit is running, and oopsing
> in those sequences is typically a lot less survivable than say oopsing in a high
> level system call while not holding locks.
>
I think what should do is have a debug option which can be set to "rw",
"log" or "oops"; the latter should probably be the default.
-hpa
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-30 21:14 ` H. Peter Anvin
@ 2015-11-30 21:33 ` Kees Cook
2015-11-30 21:38 ` Andy Lutomirski
2015-11-30 21:43 ` H. Peter Anvin
0 siblings, 2 replies; 41+ messages in thread
From: Kees Cook @ 2015-11-30 21:33 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Ingo Molnar, Andy Lutomirski, Linus Torvalds, PaX Team,
kernel-hardening, Mathias Krause, linux-kernel, Ingo Molnar,
Thomas Gleixner, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Mon, Nov 30, 2015 at 1:14 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 11/29/15 00:05, Ingo Molnar wrote:
>>
>> * Andy Lutomirski <luto@amacapital.net> wrote:
>>
>>>>> - print a warning and a backtrace, and just mark the page read-write
>>>>> so that the machine survives, but we get notified and can fix whatever
>>>>> broken code
>>>>
>>>> This seems very easy to add. Should I basically reverse the effects of
>>>> mark_rodata_ro(), or should I only make the new ro-after-init section as RW?
>>>> (I think the former would be easier.)
>>>
>>> I'd suggest verifying that the page in question is .data..ro_after_init and, if
>>> so, marking that one page RW.
>>
>> Yes, this was PaX's suggestion as well, and I agree: doing that turns a quite
>> possibly unrecoverable boot/shutdown time or suspend/resume time (suspend is
>> really a special category of 'bootup') crasher oops into a more informative stack
>> dump.
>>
>> These ro related faults tend to trigger when init/deinit is running, and oopsing
>> in those sequences is typically a lot less survivable than say oopsing in a high
>> level system call while not holding locks.
>>
>
> I think what should do is have a debug option which can be set to "rw",
> "log" or "oops"; the latter should probably be the default.
Can someone write that patch, and then I will include it in the
series? I haven't touched fault handler code, and it would be faster
if someone more familiar with that area did it. :)
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-30 21:33 ` Kees Cook
@ 2015-11-30 21:38 ` Andy Lutomirski
2015-11-30 21:43 ` H. Peter Anvin
1 sibling, 0 replies; 41+ messages in thread
From: Andy Lutomirski @ 2015-11-30 21:38 UTC (permalink / raw)
To: Kees Cook
Cc: H. Peter Anvin, Ingo Molnar, Linus Torvalds, PaX Team,
kernel-hardening, Mathias Krause, linux-kernel, Ingo Molnar,
Thomas Gleixner, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On Mon, Nov 30, 2015 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Nov 30, 2015 at 1:14 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 11/29/15 00:05, Ingo Molnar wrote:
>>>
>>> * Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>>>>> - print a warning and a backtrace, and just mark the page read-write
>>>>>> so that the machine survives, but we get notified and can fix whatever
>>>>>> broken code
>>>>>
>>>>> This seems very easy to add. Should I basically reverse the effects of
>>>>> mark_rodata_ro(), or should I only make the new ro-after-init section as RW?
>>>>> (I think the former would be easier.)
>>>>
>>>> I'd suggest verifying that the page in question is .data..ro_after_init and, if
>>>> so, marking that one page RW.
>>>
>>> Yes, this was PaX's suggestion as well, and I agree: doing that turns a quite
>>> possibly unrecoverable boot/shutdown time or suspend/resume time (suspend is
>>> really a special category of 'bootup') crasher oops into a more informative stack
>>> dump.
>>>
>>> These ro related faults tend to trigger when init/deinit is running, and oopsing
>>> in those sequences is typically a lot less survivable than say oopsing in a high
>>> level system call while not holding locks.
>>>
>>
>> I think what should do is have a debug option which can be set to "rw",
>> "log" or "oops"; the latter should probably be the default.
>
> Can someone write that patch, and then I will include it in the
> series? I haven't touched fault handler code, and it would be faster
> if someone more familiar with that area did it. :)
I think I can do it in a week or two if no one beats me to it.
--Andy
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-30 21:33 ` Kees Cook
2015-11-30 21:38 ` Andy Lutomirski
@ 2015-11-30 21:43 ` H. Peter Anvin
1 sibling, 0 replies; 41+ messages in thread
From: H. Peter Anvin @ 2015-11-30 21:43 UTC (permalink / raw)
To: Kees Cook
Cc: Ingo Molnar, Andy Lutomirski, Linus Torvalds, PaX Team,
kernel-hardening, Mathias Krause, linux-kernel, Ingo Molnar,
Thomas Gleixner, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, Emese Revfy
On 11/30/15 13:33, Kees Cook wrote:
>>
>> I think what should do is have a debug option which can be set to "rw",
>> "log" or "oops"; the latter should probably be the default.
>
> Can someone write that patch, and then I will include it in the
> series? I haven't touched fault handler code, and it would be faster
> if someone more familiar with that area did it. :)
>
The "log" option (the only hard one) can be added as a later
enhancement, and probably should be. It shouldn't block the series.
-hpa
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 9:13 ` [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory Mathias Krause
2015-11-25 10:06 ` Clemens Ladisch
2015-11-25 11:05 ` PaX Team
@ 2015-11-25 17:26 ` Kees Cook
2015-11-25 17:31 ` H. Peter Anvin
3 siblings, 0 replies; 41+ messages in thread
From: Kees Cook @ 2015-11-25 17:26 UTC (permalink / raw)
To: Mathias Krause
Cc: kernel-hardening, linux-kernel, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, x86-ml, Arnd Bergmann,
Michael Ellerman, linux-arch, PaX Team, Emese Revfy
On Wed, Nov 25, 2015 at 1:13 AM, Mathias Krause <minipli@googlemail.com> wrote:
> On 24 November 2015 at 22:38, Kees Cook <keescook@chromium.org> wrote:
>> Many things are written to only during __init, and never changed
>> again. These cannot be made "const" since the compiler will do the wrong
>> thing (we do actually need to write to them). Instead, move these items
>> into a memory region that will be made read-only during mark_rodata_ro()
>> which happens after all kernel __init code has finished.
>>
>> This introduces __read_only as a way to mark such memory, and uses it on
>> the x86 vDSO to kill an extant kernel exploitation method.
>
> ...just some random notes on the experience with kernels implementing
> such a feature for quite a lot of locations, not just the vDSO.
>
> While having that annotation makes perfect sense, not only from a
> security perspective but also from a micro-optimization point of view
> (much like the already existing __read_mostly annotation), it has its
> drawbacks. Violating the "r/o after init" rule by writing to such
> annotated variables from non-init code goes unnoticed as far as it
> concerns the toolchain. Neither the compiler nor the linker will flag
> that incorrect use. It'll just trap at runtime and that's bad.
Well, or, it's good: that's the point of it. Either from the
perspective of robustness or from that of security.
> I myself had some educating experience seeing my machine triple fault
> when resuming from a S3 sleep. The root cause was a variable that was
> annotated __read_only but that was (unnecessarily) modified during CPU
> bring-up phase. Debugging that kind of problems is sort of a PITA, you
> could imagine.
As PaX Team mentions, it should be easy to catch these traps and
report them. That could certainly be a nice addition.
> So, prior extending the usage of the __read_only annotation some
> toolchain support is needed. Maybe a gcc plugin that'll warn/error on
> code that writes to such a variable but is not __init itself. The
> initify and checker plugins from the PaX patch might be worth to look
> at for that purpose, as they're doing similar things already. Adding
> such a check to sparse might be worth it, too.
> A modpost check probably won't work as it's unable to tell if it's a
> legitimate access (r/o) or a violation (/w access). So the gcc plugin
> is the way to go, IMHO.
There are many more pieces to add to the annotation use, but I don't
want to risk getting us into a Catch-22. Emese's work on the plugin
side will see this usage and utility grow, and I think getting these
basic building blocks in place is the right place to start.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 9:13 ` [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory Mathias Krause
` (2 preceding siblings ...)
2015-11-25 17:26 ` Kees Cook
@ 2015-11-25 17:31 ` H. Peter Anvin
2015-11-25 18:54 ` Kees Cook
3 siblings, 1 reply; 41+ messages in thread
From: H. Peter Anvin @ 2015-11-25 17:31 UTC (permalink / raw)
To: Mathias Krause, kernel-hardening
Cc: linux-kernel, Kees Cook, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, x86-ml, Arnd Bergmann, Michael Ellerman,
linux-arch, PaX Team, Emese Revfy
On 11/25/15 01:13, Mathias Krause wrote:
>
> While having that annotation makes perfect sense, not only from a
> security perspective but also from a micro-optimization point of view
> (much like the already existing __read_mostly annotation), it has its
> drawbacks. Violating the "r/o after init" rule by writing to such
> annotated variables from non-init code goes unnoticed as far as it
> concerns the toolchain. Neither the compiler nor the linker will flag
> that incorrect use. It'll just trap at runtime and that's bad.
>
> I myself had some educating experience seeing my machine triple fault
> when resuming from a S3 sleep. The root cause was a variable that was
> annotated __read_only but that was (unnecessarily) modified during CPU
> bring-up phase. Debugging that kind of problems is sort of a PITA, you
> could imagine.
>
> So, prior extending the usage of the __read_only annotation some
> toolchain support is needed. Maybe a gcc plugin that'll warn/error on
> code that writes to such a variable but is not __init itself. The
> initify and checker plugins from the PaX patch might be worth to look
> at for that purpose, as they're doing similar things already. Adding
> such a check to sparse might be worth it, too.
> A modpost check probably won't work as it's unable to tell if it's a
> legitimate access (r/o) or a violation (/w access). So the gcc plugin
> is the way to go, IMHO.
>
We should not wait for compile-time support, that doesn't make any
sense. What would be useful would be a way to override this on the
command line -- that way, if disabling RO or RO-after-init memory makes
something work, we have an instant diagnosis.
-hpa
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 17:31 ` H. Peter Anvin
@ 2015-11-25 18:54 ` Kees Cook
2015-11-25 19:06 ` H. Peter Anvin
0 siblings, 1 reply; 41+ messages in thread
From: Kees Cook @ 2015-11-25 18:54 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Mathias Krause, kernel-hardening, linux-kernel, Andy Lutomirski,
Ingo Molnar, Thomas Gleixner, x86-ml, Arnd Bergmann,
Michael Ellerman, linux-arch, PaX Team, Emese Revfy
On Wed, Nov 25, 2015 at 9:31 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 11/25/15 01:13, Mathias Krause wrote:
>>
>> While having that annotation makes perfect sense, not only from a
>> security perspective but also from a micro-optimization point of view
>> (much like the already existing __read_mostly annotation), it has its
>> drawbacks. Violating the "r/o after init" rule by writing to such
>> annotated variables from non-init code goes unnoticed as far as it
>> concerns the toolchain. Neither the compiler nor the linker will flag
>> that incorrect use. It'll just trap at runtime and that's bad.
>>
>> I myself had some educating experience seeing my machine triple fault
>> when resuming from a S3 sleep. The root cause was a variable that was
>> annotated __read_only but that was (unnecessarily) modified during CPU
>> bring-up phase. Debugging that kind of problems is sort of a PITA, you
>> could imagine.
>>
>> So, prior extending the usage of the __read_only annotation some
>> toolchain support is needed. Maybe a gcc plugin that'll warn/error on
>> code that writes to such a variable but is not __init itself. The
>> initify and checker plugins from the PaX patch might be worth to look
>> at for that purpose, as they're doing similar things already. Adding
>> such a check to sparse might be worth it, too.
>> A modpost check probably won't work as it's unable to tell if it's a
>> legitimate access (r/o) or a violation (/w access). So the gcc plugin
>> is the way to go, IMHO.
>>
>
> We should not wait for compile-time support, that doesn't make any
> sense. What would be useful would be a way to override this on the
> command line -- that way, if disabling RO or RO-after-init memory makes
> something work, we have an instant diagnosis.
Seems easiest to have an arg just skip calling mark_rodata_ro(). I can add that.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [kernel-hardening] [PATCH 0/2] introduce post-init read-only memory
2015-11-25 18:54 ` Kees Cook
@ 2015-11-25 19:06 ` H. Peter Anvin
0 siblings, 0 replies; 41+ messages in thread
From: H. Peter Anvin @ 2015-11-25 19:06 UTC (permalink / raw)
To: Kees Cook
Cc: Mathias Krause, kernel-hardening, linux-kernel, Andy Lutomirski,
Ingo Molnar, Thomas Gleixner, x86-ml, Arnd Bergmann,
Michael Ellerman, linux-arch, PaX Team, Emese Revfy
On 11/25/2015 10:54 AM, Kees Cook wrote:
>>
>> We should not wait for compile-time support, that doesn't make any
>> sense. What would be useful would be a way to override this on the
>> command line -- that way, if disabling RO or RO-after-init memory makes
>> something work, we have an instant diagnosis.
>
> Seems easiest to have an arg just skip calling mark_rodata_ro(). I can add that.
>
Exactly.
-hpa
^ permalink raw reply [flat|nested] 41+ messages in thread