For a little bit of context; it turns out that user_access_begin() /
user_access_end() sets EFLAGS.AC and scheduling in between there wrecks
that because we're apparently not saving that anymore.
But only explicit scheduling — preemption and sleepy page faults are
fine because the interrupt frame saves flags.
No, like pointed out elsewhere in this thread, anything that does
preempt_disable() is utterly broken with this.
Because at that point the IRQ return path doesn't reschedule but
preempt_enable() will, and that doesn't preserve EFLAGS again.
quoted
Now, I'm tempted to add the PUSHF / POPF right back because of this, but
first I suppose we need to figure out if that change was on purpose and
why that went missing from the Changelog.
That’s certainly the easy solution. Or we could teach the might_sleep
checks about this, but that could be a mess.
That's not enough, we'd have to teach preempt_disable(), but worse,
preempt_disable_notrace().
Anything that lands in ftrace, which _will_ use
preempt_disable_notrace(), will screw this thing up.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
For a little bit of context; it turns out that user_access_begin() /
user_access_end() sets EFLAGS.AC and scheduling in between there wrecks
that because we're apparently not saving that anymore.
But only explicit scheduling — preemption and sleepy page faults are
fine because the interrupt frame saves flags.
No, like pointed out elsewhere in this thread, anything that does
preempt_disable() is utterly broken with this.
Because at that point the IRQ return path doesn't reschedule but
preempt_enable() will, and that doesn't preserve EFLAGS again.
quoted
quoted
Now, I'm tempted to add the PUSHF / POPF right back because of this, but
first I suppose we need to figure out if that change was on purpose and
why that went missing from the Changelog.
That’s certainly the easy solution. Or we could teach the might_sleep
checks about this, but that could be a mess.
That's not enough, we'd have to teach preempt_disable(), but worse,
preempt_disable_notrace().
Anything that lands in ftrace, which _will_ use
preempt_disable_notrace(), will screw this thing up.
Ugh. Consider your patch acked. Do we need to backport this thing? The problem can’t be too widespread or we would have heard of it before.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-14 10:14:45
On Wed, Feb 13, 2019 at 02:49:47PM -0800, Andy Lutomirski wrote:
Do we need to backport this thing?
Possibly, just to be safe.
The problem can’t be too widespread or we would have heard of it before.
Yes, so far we've been lucky.
---
Subject: sched/x86: Save [ER]FLAGS on context switch
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 14 10:30:52 CET 2019
Effectively revert commit:
2c7577a75837 ("sched/x86_64: Don't save flags on context switch")
Specifically because SMAP uses FLAGS.AC which invalidates the claim
that the kernel has clean flags.
In particular; while preemption from interrupt return is fine (the
IRET frame on the exception stack contains FLAGS) it breaks any code
that does synchonous scheduling, including preempt_enable().
This has become a significant issue ever since commit:
5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
provided for means of having 'normal' C code between STAC / CLAC,
exposing the FLAGS.AC state. So far this hasn't led to trouble,
however fix it before it comes apart.
Fixes: 5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
Acked-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Julien Thierry <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/entry_32.S | 2 ++
arch/x86/entry/entry_64.S | 2 ++
arch/x86/include/asm/switch_to.h | 1 +
3 files changed, 5 insertions(+)
From: Brian Gerst <hidden> Date: 2019-02-14 16:18:17
On Thu, Feb 14, 2019 at 5:14 AM Peter Zijlstra [off-list ref] wrote:
quoted hunk
On Wed, Feb 13, 2019 at 02:49:47PM -0800, Andy Lutomirski wrote:
quoted
Do we need to backport this thing?
Possibly, just to be safe.
quoted
The problem can’t be too widespread or we would have heard of it before.
Yes, so far we've been lucky.
---
Subject: sched/x86: Save [ER]FLAGS on context switch
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 14 10:30:52 CET 2019
Effectively revert commit:
2c7577a75837 ("sched/x86_64: Don't save flags on context switch")
Specifically because SMAP uses FLAGS.AC which invalidates the claim
that the kernel has clean flags.
In particular; while preemption from interrupt return is fine (the
IRET frame on the exception stack contains FLAGS) it breaks any code
that does synchonous scheduling, including preempt_enable().
This has become a significant issue ever since commit:
5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
provided for means of having 'normal' C code between STAC / CLAC,
exposing the FLAGS.AC state. So far this hasn't led to trouble,
however fix it before it comes apart.
Fixes: 5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
Acked-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Julien Thierry <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/entry_32.S | 2 ++
arch/x86/entry/entry_64.S | 2 ++
arch/x86/include/asm/switch_to.h | 1 +
3 files changed, 5 insertions(+)
flags should be initialized in copy_thread_tls(). I think the new
stack is zeroed already, but it would be better to explicitly set it.
--
Brian Gerst
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -127,6 +127,7 @@ int copy_thread_tls(unsigned long clone_structtask_struct*tsk;interr;+frame->flags=0;frame->bp=0;frame->ret_addr=(unsignedlong)ret_from_fork;p->thread.sp=(unsignedlong)fork_frame;---a/arch/x86/kernel/process_64.c+++b/arch/x86/kernel/process_64.c
@@ -392,6 +392,7 @@ int copy_thread_tls(unsigned long clone_childregs=task_pt_regs(p);fork_frame=container_of(childregs,structfork_frame,regs);frame=&fork_frame->frame;+frame->flags=0;frame->bp=0;frame->ret_addr=(unsignedlong)ret_from_fork;p->thread.sp=(unsignedlong)fork_frame;
@@ -127,6 +127,7 @@ int copy_thread_tls(unsigned long clone_structtask_struct*tsk;interr;+frame->flags=0;frame->bp=0;frame->ret_addr=(unsignedlong)ret_from_fork;p->thread.sp=(unsignedlong)fork_frame;---a/arch/x86/kernel/process_64.c+++b/arch/x86/kernel/process_64.c
@@ -392,6 +392,7 @@ int copy_thread_tls(unsigned long clone_childregs=task_pt_regs(p);fork_frame=container_of(childregs,structfork_frame,regs);frame=&fork_frame->frame;+frame->flags=0;frame->bp=0;frame->ret_addr=(unsignedlong)ret_from_fork;p->thread.sp=(unsignedlong)fork_frame;
On Thu, Feb 14, 2019 at 11:34 AM Peter Zijlstra [off-list ref] wrote:
Something like the below, right?
+ frame->flags = 0;
+ frame->flags = 0;
Those are not valid flag values.
Can you popf them? Yes.
Do they make sense? No.
It has the IF flag clear, for example. Is that intentional? If it is,
it should likely be documented. Because IF clear means "interrupts
disabled". Are the places that load flags in irq disabled regions?
Maybe they are, maybe they aren't, but shouldn't this be something
that is made explicit, rather than "oh, let's initialize to zero like
all the other registers we don't care about".
Because a zero initializer for eflags really is odd.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-15 17:40:56
On Fri, Feb 15, 2019 at 09:18:00AM -0800, Linus Torvalds wrote:
On Thu, Feb 14, 2019 at 11:34 AM Peter Zijlstra [off-list ref] wrote:
quoted
Something like the below, right?
+ frame->flags = 0;
+ frame->flags = 0;
Those are not valid flag values.
Can you popf them? Yes.
Do they make sense? No.
It has the IF flag clear, for example. Is that intentional? If it is,
Uhmm. yeah, that's bonkers. We should have interrupts disabled here.
I'll go read up on the eflags and figure out what they _should_ be right
about there.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Lutomirski <luto@amacapital.net> Date: 2019-02-15 18:28:13
On Feb 15, 2019, at 9:40 AM, Peter Zijlstra [off-list ref] wrote:
quoted
On Fri, Feb 15, 2019 at 09:18:00AM -0800, Linus Torvalds wrote:
quoted
On Thu, Feb 14, 2019 at 11:34 AM Peter Zijlstra [off-list ref] wrote:
Something like the below, right?
+ frame->flags = 0;
+ frame->flags = 0;
Those are not valid flag values.
Can you popf them? Yes.
Do they make sense? No.
It has the IF flag clear, for example. Is that intentional? If it is,
Uhmm. yeah, that's bonkers. We should have interrupts disabled here.
I'll go read up on the eflags and figure out what they _should_ be right
about there.
And probably add a comment near the POPF explaining that it will keep IRQs off :)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-15 23:34:40
On Fri, Feb 15, 2019 at 06:40:34PM +0100, Peter Zijlstra wrote:
On Fri, Feb 15, 2019 at 09:18:00AM -0800, Linus Torvalds wrote:
quoted
On Thu, Feb 14, 2019 at 11:34 AM Peter Zijlstra [off-list ref] wrote:
quoted
Something like the below, right?
+ frame->flags = 0;
+ frame->flags = 0;
Those are not valid flag values.
Can you popf them? Yes.
Do they make sense? No.
It has the IF flag clear, for example. Is that intentional? If it is,
Uhmm. yeah, that's bonkers. We should have interrupts disabled here.
I'll go read up on the eflags and figure out what they _should_ be right
about there.
I misread (I'm forever confused about what way around IF goes), but you
said it right; IF=0 is interrupts disabled and we very much have that in
the middle of context switch.
(just for giggles I set IF for the initial flags value; and it comes
unstuck _real_ quick)
Now, EFLAGS bit 1 is supposedly always 1, but it really doesn't seem to
matter for POPF.
I went through the other flags, and aside from VIP/VIF (I've no clue),
they looks like 0 should be just fine.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Feb 15, 2019 at 3:34 PM Peter Zijlstra [off-list ref] wrote:
Now, EFLAGS bit 1 is supposedly always 1, but it really doesn't seem to
matter for POPF.
Correct, it's "read as 1", you can try to write it and it doesn't matter.
I went through the other flags, and aside from VIP/VIF (I've no clue),
they looks like 0 should be just fine.
So 0 is a perfectly valid initializer in the sense that it _works_, I
just want it to be something that was thought about, not just a random
"initialize to zero" without thinking.
Even just a comment about it would be fine. But it might also be good
to show that it's an explicit eflags value and just use
X86_EFLAGS_FIXED as the initializer.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On February 14, 2019 2:14:29 AM PST, Peter Zijlstra [off-list ref] wrote:
quoted hunk
On Wed, Feb 13, 2019 at 02:49:47PM -0800, Andy Lutomirski wrote:
quoted
Do we need to backport this thing?
Possibly, just to be safe.
quoted
The problem can’t be too widespread or we would have heard of it
before.
Yes, so far we've been lucky.
---
Subject: sched/x86: Save [ER]FLAGS on context switch
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 14 10:30:52 CET 2019
Effectively revert commit:
2c7577a75837 ("sched/x86_64: Don't save flags on context switch")
Specifically because SMAP uses FLAGS.AC which invalidates the claim
that the kernel has clean flags.
In particular; while preemption from interrupt return is fine (the
IRET frame on the exception stack contains FLAGS) it breaks any code
that does synchonous scheduling, including preempt_enable().
This has become a significant issue ever since commit:
5b24a7a2aa20 ("Add 'unsafe' user access functions for batched
accesses")
provided for means of having 'normal' C code between STAC / CLAC,
exposing the FLAGS.AC state. So far this hasn't led to trouble,
however fix it before it comes apart.
Fixes: 5b24a7a2aa20 ("Add 'unsafe' user access functions for batched
accesses")
Acked-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Julien Thierry <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/entry_32.S | 2 ++
arch/x86/entry/entry_64.S | 2 ++
arch/x86/include/asm/switch_to.h | 1 +
3 files changed, 5 insertions(+)
struct inactive_task_frame {
+ unsigned long flags;
#ifdef CONFIG_X86_64
unsigned long r15;
unsigned long r14;
This implies we invoke schedule -- a restricted operation (consider may_sleep) during execution of STAC-enabled code, but *not* as an exception or interrupt, since those preserve the flags.
I have serious concerns about this. This is more or less saying that we have left an unlimited gap and have had AC escape.
Does *anyone* see a need to allow this? I got a question at LPC from someone about this, and what they were trying to do once all the layers had been unwound was so far down the wrong track for a root problem that actually has a very simple solution.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-16 10:31:03
On Fri, Feb 15, 2019 at 08:06:56PM -0800, hpa@zytor.com wrote:
This implies we invoke schedule -- a restricted operation (consider
may_sleep) during execution of STAC-enabled code, but *not* as an
exception or interrupt, since those preserve the flags.
Meet preempt_enable().
I have serious concerns about this. This is more or less saying that
we have left an unlimited gap and have had AC escape.
Yes; by allowing normal C in between STAC and CLAC this is so.
Does *anyone* see a need to allow this? I got a question at LPC from
someone about this, and what they were trying to do once all the
layers had been unwound was so far down the wrong track for a root
problem that actually has a very simple solution.
Have you read the rest of the thread?
All it takes for this to explode is a call to a C function that has
tracing on it in between the user_access_begin() and user_access_end()
things. That is a _very_ easy thing to do.
Heck, GCC is allowed to generate that broken code compiling
lib/strnlen_user.c; it doesn't _have_ to inline do_strnlen_user (esp.
with CONFIG_OPTIMIZE_INLINING), and making that a function call would
get us fentry hooks and ftrace and *BOOM*.
(Now, of course, its a static function with a single caller, and GCC
isn't _THAT_ stupid, but it could, if it wanted to)
Since the bar is _that_ low for mistakes, I figure we'd better fix it.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-16 10:32:55
On Fri, Feb 15, 2019 at 04:21:46PM -0800, Linus Torvalds wrote:
Even just a comment about it would be fine. But it might also be good
to show that it's an explicit eflags value and just use
X86_EFLAGS_FIXED as the initializer.
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-18 09:03:47
Subject: sched/x86: Save [ER]FLAGS on context switch
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 14 10:30:52 CET 2019
Effectively reverts commit:
2c7577a75837 ("sched/x86_64: Don't save flags on context switch")
Specifically because SMAP uses FLAGS.AC which invalidates the claim
that the kernel has clean flags.
In particular; while preemption from interrupt return is fine (the
IRET frame on the exception stack contains FLAGS) it breaks any code
that does synchonous scheduling, including preempt_enable().
This has become a significant issue ever since commit:
5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
provided for means of having 'normal' C code between STAC / CLAC,
exposing the FLAGS.AC state. So far this hasn't led to trouble,
however fix it before it comes apart.
Fixes: 5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
Acked-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Julien Thierry <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/entry_32.S | 2 ++
arch/x86/entry/entry_64.S | 2 ++
arch/x86/include/asm/switch_to.h | 1 +
arch/x86/kernel/process_32.c | 7 +++++++
arch/x86/kernel/process_64.c | 8 ++++++++
5 files changed, 20 insertions(+)
@@ -127,6 +127,13 @@ int copy_thread_tls(unsigned long clone_structtask_struct*tsk;interr;+/*+*ForanewtaskusetheRESETflagsvaluesincethereisnobefore.+*Allthestatusflagsarezero;DFandallthesystemflagsmustalso+*be0,specificallyIFmustbe0becausewecontextswitchtothenew+*taskwithinterruptsdisabled.+*/+frame->flags=X86_EFLAGS_FIXED;frame->bp=0;frame->ret_addr=(unsignedlong)ret_from_fork;p->thread.sp=(unsignedlong)fork_frame;---a/arch/x86/kernel/process_64.c+++b/arch/x86/kernel/process_64.c
@@ -392,6 +392,14 @@ int copy_thread_tls(unsigned long clone_childregs=task_pt_regs(p);fork_frame=container_of(childregs,structfork_frame,regs);frame=&fork_frame->frame;++/*+*ForanewtaskusetheRESETflagsvaluesincethereisnobefore.+*Allthestatusflagsarezero;DFandallthesystemflagsmustalso+*be0,specificallyIFmustbe0becausewecontextswitchtothenew+*taskwithinterruptsdisabled.+*/+frame->flags=X86_EFLAGS_FIXED;frame->bp=0;frame->ret_addr=(unsignedlong)ret_from_fork;p->thread.sp=(unsignedlong)fork_frame;
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2019-02-18 22:31:06
On 2/16/19 2:30 AM, Peter Zijlstra wrote:
On Fri, Feb 15, 2019 at 08:06:56PM -0800, hpa@zytor.com wrote:
quoted
This implies we invoke schedule -- a restricted operation (consider
may_sleep) during execution of STAC-enabled code, but *not* as an
exception or interrupt, since those preserve the flags.
Meet preempt_enable().
I believe this falls under "doctor, it hurts when I do that." And it hurts for
very good reasons. See below.
quoted
I have serious concerns about this. This is more or less saying that
we have left an unlimited gap and have had AC escape.
Yes; by allowing normal C in between STAC and CLAC this is so.
quoted
Does *anyone* see a need to allow this? I got a question at LPC from
someone about this, and what they were trying to do once all the
layers had been unwound was so far down the wrong track for a root
problem that actually has a very simple solution.
Have you read the rest of the thread?
All it takes for this to explode is a call to a C function that has
tracing on it in between the user_access_begin() and user_access_end()
things. That is a _very_ easy thing to do.
Heck, GCC is allowed to generate that broken code compiling
lib/strnlen_user.c; it doesn't _have_ to inline do_strnlen_user (esp.
with CONFIG_OPTIMIZE_INLINING), and making that a function call would
get us fentry hooks and ftrace and *BOOM*.
(Now, of course, its a static function with a single caller, and GCC
isn't _THAT_ stupid, but it could, if it wanted to)
Since the bar is _that_ low for mistakes, I figure we'd better fix it.
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
For an issue specific to tracing, it would be more appropriate to do the
appropriate things in the tracing entry/exit than in schedule. Otherwise, we
don't want to silently paper over mistakes which could mean that we run a
large portion of the kernel without protection we thought we had.
In that sense, calling schedule() with AC set is in fact a great place to have
a WARN() or even BUG(), because such an event really could imply that the
kernel has been security compromised.
Does that make more sense?
-hpa
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
We'd just have to abstract it sanely. I'm sure arm64 has the exact
same issue with PAN - maybe it saves properly, but the same "we
wouldn't want to go through the scheduler with PAN clear".
On x86, we might as well check DF at the same time as AC.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Lutomirski <luto@amacapital.net> Date: 2019-02-19 02:20:12
On Feb 18, 2019, at 4:24 PM, Linus Torvalds [off-list ref] wrote:
quoted
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
We'd just have to abstract it sanely. I'm sure arm64 has the exact
same issue with PAN - maybe it saves properly, but the same "we
wouldn't want to go through the scheduler with PAN clear".
On x86, we might as well check DF at the same time as AC.
hpa is right, though — calling into tracing code with AC set is not really so good. And calling schedule() (via preempt_enable() or whatever) is also bad because it runs all the scheduler code with AC on. Admittedly, the scheduler is not *that* interesting of an attack surface.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2019-02-19 02:46:45
On 2/18/19 6:20 PM, Andy Lutomirski wrote:
quoted
On Feb 18, 2019, at 4:24 PM, Linus Torvalds [off-list ref] wrote:
quoted
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
We'd just have to abstract it sanely. I'm sure arm64 has the exact
same issue with PAN - maybe it saves properly, but the same "we
wouldn't want to go through the scheduler with PAN clear".
On x86, we might as well check DF at the same time as AC.
hpa is right, though — calling into tracing code with AC set is not really so good. And calling schedule() (via preempt_enable() or whatever) is also bad because it runs all the scheduler code with AC on. Admittedly, the scheduler is not *that* interesting of an attack surface.
Not just that, but the other question is just how much code we are running
with AC open. It really should only be done in some very small regions.
-hpa
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
quoted
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
We'd just have to abstract it sanely. I'm sure arm64 has the exact
same issue with PAN - maybe it saves properly, but the same "we
wouldn't want to go through the scheduler with PAN clear".
As of right now, we have the same issue on arm64 as on x86. We don't
currently save the PAN bit on task switch, but I have a patch to do that.
Unless we decide to go down the route of warning against uses of
schedule() inside.
As for the abstraction, I had this patch[1] that added another primitive
for the user_access API (although this might not be suited for x86 if
you also want to check DF). However, an issue that appears is where to
perform the check to cover enough ground.
Checking inside the schedule() code you only cover cases where things
have already gone wrong, and not the use of functions that are unsafe to
call inside a user_access region.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/625385.html
Cheers,
--
Julien Thierry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-19 09:04:25
On Mon, Feb 18, 2019 at 02:30:21PM -0800, H. Peter Anvin wrote:
On 2/16/19 2:30 AM, Peter Zijlstra wrote:
quoted
On Fri, Feb 15, 2019 at 08:06:56PM -0800, hpa@zytor.com wrote:
quoted
This implies we invoke schedule -- a restricted operation (consider
may_sleep) during execution of STAC-enabled code, but *not* as an
exception or interrupt, since those preserve the flags.
Meet preempt_enable().
I believe this falls under "doctor, it hurts when I do that." And it hurts for
very good reasons. See below.
I disagree; the basic rule is that if you're preemptible you must also
be able to schedule and vice-versa. These AC regions violate this.
And, like illustrated, it is _very_ easy to get all sorts inside that AC
crap.
quoted
quoted
I have serious concerns about this. This is more or less saying that
we have left an unlimited gap and have had AC escape.
Yes; by allowing normal C in between STAC and CLAC this is so.
quoted
Does *anyone* see a need to allow this? I got a question at LPC from
someone about this, and what they were trying to do once all the
layers had been unwound was so far down the wrong track for a root
problem that actually has a very simple solution.
Have you read the rest of the thread?
All it takes for this to explode is a call to a C function that has
tracing on it in between the user_access_begin() and user_access_end()
things. That is a _very_ easy thing to do.
Heck, GCC is allowed to generate that broken code compiling
lib/strnlen_user.c; it doesn't _have_ to inline do_strnlen_user (esp.
with CONFIG_OPTIMIZE_INLINING), and making that a function call would
get us fentry hooks and ftrace and *BOOM*.
(Now, of course, its a static function with a single caller, and GCC
isn't _THAT_ stupid, but it could, if it wanted to)
Since the bar is _that_ low for mistakes, I figure we'd better fix it.
The question is what "fix it" means.
It means that when we do schedule, the next task doesn't have AC set,
and when we schedule back, we'll restore our AC when we had it. Unlike
the current situation, where the next task will run with AC set.
IOW, it confines AC to the task context where it was set.
I'm really concerned about AC escapes,
and everyone else should be, too.
Then _that_ should be asserted.
For an issue specific to tracing, it would be more appropriate to do the
appropriate things in the tracing entry/exit than in schedule. Otherwise, we
don't want to silently paper over mistakes which could mean that we run a
large portion of the kernel without protection we thought we had.
In that sense, calling schedule() with AC set is in fact a great place to have
a WARN() or even BUG(), because such an event really could imply that the
kernel has been security compromised.
It is not specific to tracing, tracing is just one of the most trivial
and non-obvious ways to make it go splat.
There's lot of fairly innocent stuff that does preempt_disable() /
preempt_enable(). And just a warning in schedule() isn't sufficient,
you'd have to actually trigger a reschedule before you know your code is
bad.
And like I said; the invariant is: if you're preemptible you can
schedule and v.v.
Now, clearly you don't want to mark these whole regions as !preemptible,
because then you can also not take faults, but somehow you're not
worried about the whole fault handler, but you are worried about the
scheduler ?!? How does that work? The fault handler can touch a _ton_
more code. Heck, the fault handler can schedule.
So either pre-fault, and run the whole AC crap with preemption disabled
and retry, or accept that we have to schedule.
On Feb 18, 2019, at 4:24 PM, Linus Torvalds [off-list ref] wrote:
quoted
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
We'd just have to abstract it sanely. I'm sure arm64 has the exact
same issue with PAN - maybe it saves properly, but the same "we
wouldn't want to go through the scheduler with PAN clear".
On x86, we might as well check DF at the same time as AC.
hpa is right, though — calling into tracing code with AC set is not really so good. And calling schedule() (via preempt_enable() or whatever) is also bad because it runs all the scheduler code with AC on. Admittedly, the scheduler is not *that* interesting of an attack surface.
Not just that, but the other question is just how much code we are running
with AC open. It really should only be done in some very small regions.
Yes, but we don't really have a way to enforce that, as far as I'm aware.
The user_access_begin/end() is generic API, meaning any arch is free to
implement it. If they don't have the same hardware behaviour as
x86/arm64, it might be that their interrupt/exception entry code will
run with user_access open until they reach the entry code that closes it
(and entry code could potentially be a more interesting attack surface
than the scheduler). This could be the case of software emulated PAN on
arm/arm64 (although currently arm, non-64bit, doesn't have
user_access_begin/end() at the time).
So the whole "very small region" restriction sounds a bit
loose/arbitrary to me...
Thanks,
--
Julien Thierry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-19 09:15:37
On Mon, Feb 18, 2019 at 04:24:30PM -0800, Linus Torvalds wrote:
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
quoted
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
So I disagree.
Either we set AC with preempt disabled, and then we don't need an extra
warning, because we already have a warning about scheduling with
preemption disabled, or we accept that the fault handler can run.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-19 09:19:34
On Tue, Feb 19, 2019 at 10:15:25AM +0100, Peter Zijlstra wrote:
On Mon, Feb 18, 2019 at 04:24:30PM -0800, Linus Torvalds wrote:
quoted
On Mon, Feb 18, 2019 at 2:31 PM H. Peter Anvin [off-list ref] wrote:
quoted
The question is what "fix it" means. I'm really concerned about AC escapes,
and everyone else should be, too.
I do think that it might be the right thing to do to add some kind of
WARN_ON_ONCE() for AC being set in various can-reschedule situations.
So I disagree.
Either we set AC with preempt disabled, and then we don't need an extra
warning, because we already have a warning about scheduling with
preemption disabled, or we accept that the fault handler can run.
n/m about the faults, forgot the obvious :/
I still really dislike wrecking the preemption model over this.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On February 19, 2019 1:04:09 AM PST, Peter Zijlstra [off-list ref] wrote:
On Mon, Feb 18, 2019 at 02:30:21PM -0800, H. Peter Anvin wrote:
quoted
On 2/16/19 2:30 AM, Peter Zijlstra wrote:
quoted
On Fri, Feb 15, 2019 at 08:06:56PM -0800, hpa@zytor.com wrote:
quoted
This implies we invoke schedule -- a restricted operation
(consider
quoted
quoted
quoted
may_sleep) during execution of STAC-enabled code, but *not* as an
exception or interrupt, since those preserve the flags.
Meet preempt_enable().
I believe this falls under "doctor, it hurts when I do that." And it
hurts for
quoted
very good reasons. See below.
I disagree; the basic rule is that if you're preemptible you must also
be able to schedule and vice-versa. These AC regions violate this.
And, like illustrated, it is _very_ easy to get all sorts inside that
AC
crap.
quoted
quoted
quoted
I have serious concerns about this. This is more or less saying
that
quoted
quoted
quoted
we have left an unlimited gap and have had AC escape.
Yes; by allowing normal C in between STAC and CLAC this is so.
quoted
Does *anyone* see a need to allow this? I got a question at LPC
from
quoted
quoted
quoted
someone about this, and what they were trying to do once all the
layers had been unwound was so far down the wrong track for a root
problem that actually has a very simple solution.
Have you read the rest of the thread?
All it takes for this to explode is a call to a C function that has
tracing on it in between the user_access_begin() and
user_access_end()
quoted
quoted
things. That is a _very_ easy thing to do.
Heck, GCC is allowed to generate that broken code compiling
lib/strnlen_user.c; it doesn't _have_ to inline do_strnlen_user
(esp.
quoted
quoted
with CONFIG_OPTIMIZE_INLINING), and making that a function call
would
quoted
quoted
get us fentry hooks and ftrace and *BOOM*.
(Now, of course, its a static function with a single caller, and
GCC
quoted
quoted
isn't _THAT_ stupid, but it could, if it wanted to)
Since the bar is _that_ low for mistakes, I figure we'd better fix
it.
quoted
quoted
The question is what "fix it" means.
It means that when we do schedule, the next task doesn't have AC set,
and when we schedule back, we'll restore our AC when we had it. Unlike
the current situation, where the next task will run with AC set.
IOW, it confines AC to the task context where it was set.
quoted
I'm really concerned about AC escapes,
and everyone else should be, too.
Then _that_ should be asserted.
quoted
For an issue specific to tracing, it would be more appropriate to do
the
quoted
appropriate things in the tracing entry/exit than in schedule.
Otherwise, we
quoted
don't want to silently paper over mistakes which could mean that we
run a
quoted
large portion of the kernel without protection we thought we had.
In that sense, calling schedule() with AC set is in fact a great
place to have
quoted
a WARN() or even BUG(), because such an event really could imply that
the
quoted
kernel has been security compromised.
It is not specific to tracing, tracing is just one of the most trivial
and non-obvious ways to make it go splat.
There's lot of fairly innocent stuff that does preempt_disable() /
preempt_enable(). And just a warning in schedule() isn't sufficient,
you'd have to actually trigger a reschedule before you know your code
is
bad.
And like I said; the invariant is: if you're preemptible you can
schedule and v.v.
Now, clearly you don't want to mark these whole regions as
!preemptible,
because then you can also not take faults, but somehow you're not
worried about the whole fault handler, but you are worried about the
scheduler ?!? How does that work? The fault handler can touch a _ton_
more code. Heck, the fault handler can schedule.
So either pre-fault, and run the whole AC crap with preemption disabled
and retry, or accept that we have to schedule.
quoted
Does that make more sense?
It appears to me you're going about it backwards.
I'm not worried about the fault handler, because AC is always presented/disabled on exception entry; otherwise I would of course be very concerned.
To be clear: I'm not worried about the scheduler itself. I'm worried about how much code we have gone through to get there. Perhaps the scheduler itself is not the right point to probe for it, but we do need to catch things that have gone wrong, rather than just leaving the door wide open.
I would personally be far more comfortable saying you have to disable preemption in user access regions. It may be an unnecessary constraint for x86 and ARM64, but it is safe.
And Julien, yes, it is "somewhat arbitrary", but so are many engineering tradeoffs. Not everything has a very sharply definable line.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-19 09:45:00
On Tue, Feb 19, 2019 at 10:04:09AM +0100, Peter Zijlstra wrote:
quoted
Does that make more sense?
It appears to me you're going about it backwards.
So how about you do a GCC plugin that verifies limits on code-gen
between user_access_begin/user_access_end() ?
- No CALL/RET
- implies user_access_end() happens
- implies no fentry hooks
- No __preempt_count frobbing
- No tracepoints
- ...
That way you put the burden on the special code, not on the rest of the
kernel.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Thomas Gleixner <hidden> Date: 2019-02-19 11:38:53
On Tue, 19 Feb 2019, Peter Zijlstra wrote:
On Tue, Feb 19, 2019 at 10:04:09AM +0100, Peter Zijlstra wrote:
quoted
quoted
Does that make more sense?
It appears to me you're going about it backwards.
So how about you do a GCC plugin that verifies limits on code-gen
between user_access_begin/user_access_end() ?
- No CALL/RET
- implies user_access_end() happens
- implies no fentry hooks
- No __preempt_count frobbing
- No tracepoints
- ...
That way you put the burden on the special code, not on the rest of the
kernel.
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-19 11:58:55
On Tue, Feb 19, 2019 at 12:38:42PM +0100, Thomas Gleixner wrote:
On Tue, 19 Feb 2019, Peter Zijlstra wrote:
quoted
On Tue, Feb 19, 2019 at 10:04:09AM +0100, Peter Zijlstra wrote:
quoted
quoted
Does that make more sense?
It appears to me you're going about it backwards.
So how about you do a GCC plugin that verifies limits on code-gen
between user_access_begin/user_access_end() ?
- No CALL/RET
- implies user_access_end() happens
- implies no fentry hooks
- No __preempt_count frobbing
- No tracepoints
- ...
That way you put the burden on the special code, not on the rest of the
kernel.
And then you have kprobes ....
They prod the INT3 byte and then take an exception, and exceptions are
'fine'.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <hidden> Date: 2019-02-19 12:48:18
On Tue, Feb 19, 2019 at 10:04:09AM +0100, Peter Zijlstra wrote:
On Mon, Feb 18, 2019 at 02:30:21PM -0800, H. Peter Anvin wrote:
quoted
On 2/16/19 2:30 AM, Peter Zijlstra wrote:
quoted
On Fri, Feb 15, 2019 at 08:06:56PM -0800, hpa@zytor.com wrote:
quoted
This implies we invoke schedule -- a restricted operation (consider
may_sleep) during execution of STAC-enabled code, but *not* as an
exception or interrupt, since those preserve the flags.
Meet preempt_enable().
I believe this falls under "doctor, it hurts when I do that." And it hurts for
very good reasons. See below.
I disagree; the basic rule is that if you're preemptible you must also
be able to schedule and vice-versa. These AC regions violate this.
And, like illustrated, it is _very_ easy to get all sorts inside that AC
crap.
quoted
quoted
quoted
I have serious concerns about this. This is more or less saying that
we have left an unlimited gap and have had AC escape.
Yes; by allowing normal C in between STAC and CLAC this is so.
quoted
Does *anyone* see a need to allow this? I got a question at LPC from
someone about this, and what they were trying to do once all the
layers had been unwound was so far down the wrong track for a root
problem that actually has a very simple solution.
Have you read the rest of the thread?
All it takes for this to explode is a call to a C function that has
tracing on it in between the user_access_begin() and user_access_end()
things. That is a _very_ easy thing to do.
Heck, GCC is allowed to generate that broken code compiling
lib/strnlen_user.c; it doesn't _have_ to inline do_strnlen_user (esp.
with CONFIG_OPTIMIZE_INLINING), and making that a function call would
get us fentry hooks and ftrace and *BOOM*.
(Now, of course, its a static function with a single caller, and GCC
isn't _THAT_ stupid, but it could, if it wanted to)
Since the bar is _that_ low for mistakes, I figure we'd better fix it.
The question is what "fix it" means.
It means that when we do schedule, the next task doesn't have AC set,
and when we schedule back, we'll restore our AC when we had it. Unlike
the current situation, where the next task will run with AC set.
IOW, it confines AC to the task context where it was set.
quoted
I'm really concerned about AC escapes,
and everyone else should be, too.
Then _that_ should be asserted.
quoted
For an issue specific to tracing, it would be more appropriate to do the
appropriate things in the tracing entry/exit than in schedule. Otherwise, we
don't want to silently paper over mistakes which could mean that we run a
large portion of the kernel without protection we thought we had.
In that sense, calling schedule() with AC set is in fact a great place to have
a WARN() or even BUG(), because such an event really could imply that the
kernel has been security compromised.
It is not specific to tracing, tracing is just one of the most trivial
and non-obvious ways to make it go splat.
There's lot of fairly innocent stuff that does preempt_disable() /
preempt_enable(). And just a warning in schedule() isn't sufficient,
you'd have to actually trigger a reschedule before you know your code is
bad.
And like I said; the invariant is: if you're preemptible you can
schedule and v.v.
Now, clearly you don't want to mark these whole regions as !preemptible,
because then you can also not take faults, but somehow you're not
worried about the whole fault handler, but you are worried about the
scheduler ?!? How does that work? The fault handler can touch a _ton_
more code. Heck, the fault handler can schedule.
So either pre-fault, and run the whole AC crap with preemption disabled
and retry, or accept that we have to schedule.
I think you'll still hate this, but could we not disable preemption during
the uaccess-enabled region, re-enabling it on the fault path after we've
toggled uaccess off and disable it again when we return back to the
uaccess-enabled region? Doesn't help with tracing, but it should at least
handle the common case.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2019-02-20 22:56:38
On 2/19/19 4:48 AM, Will Deacon wrote:
I think you'll still hate this, but could we not disable preemption during
the uaccess-enabled region, re-enabling it on the fault path after we've
toggled uaccess off and disable it again when we return back to the
uaccess-enabled region? Doesn't help with tracing, but it should at least
handle the common case.
There is a worse problem with this, I still realize: this would mean blocking
preemption across what could possibly be a *very* large copy_from_user(), for
example.
Exceptions *have* to handle this; there is no way around it. Perhaps the
scheduler isn't the right place to put these kinds of asserts, either.
Now, __fentry__ is kind of a special beast; in some ways it is an "exception
implemented as a function call"; on x86 one could even consider using an INT
instruction in order to reduce the NOP footprint in the unarmed case. Nor is
__fentry__ a C function; it has far more of an exception-like ABI.
*Regardless* of what else we do, I believe __fentry__ ought to
save/disable/restore AC, just like an exception does.
The idea of using s/a gcc plugin/objtool/ for this isn't really a bad idea.
Obviously the general problem is undecidable :) but the enforcement of some
simple, fairly draconian rules ("as tight as possible, but no tighter")
shouldn't be a huge problem.
An actual gcc plugin -- which would probably be quite complex -- could make
gcc itself aware of user space accesses and be able to rearrange them to
minimize STAC/CLAC and avoid kernel-space accesses inside those brackets.
Finally, of course, there is the option of simply outlawing this practice as a
matter of policy and require that all structures be accessed through a limited
set of APIs. As I recall, the number of places where there were
performance-critical regions which could not use the normal accessors are
fairly small (signal frames being the main one.) Doing bulk copy to/from
kernel memory and then accessing them from there would have some performance
cost, but would eliminate the need for this complexity entirely.
-hpa
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
I think you'll still hate this, but could we not disable preemption during
the uaccess-enabled region, re-enabling it on the fault path after we've
toggled uaccess off and disable it again when we return back to the
uaccess-enabled region? Doesn't help with tracing, but it should at least
handle the common case.
There is a worse problem with this, I still realize: this would mean blocking
preemption across what could possibly be a *very* large copy_from_user(), for
example.
Yes, that's a good point. And I guess a userspace program could forcibly
trigger the kernel into a large copy_from/to_user(), knowingly disabling
preemption.
I don't know how bad this could get.
Exceptions *have* to handle this; there is no way around it. Perhaps the
scheduler isn't the right place to put these kinds of asserts, either.
Maybe I'm misunderstanding what you mean with "Exceptions *have* to
handle this". Isn't the fact that the AC/PAN flags gets saved on
exception entry and set back to "user accesses disabled" already
handling it? Or are you referring to something else?
So far my understanding is that the exception/interrupt case is fine.
The worrying case is what gets called in the user access regions
(schedule(), preempt_enable(), tracing...).
Did I get lost somewhere?
Now, __fentry__ is kind of a special beast; in some ways it is an "exception
implemented as a function call"; on x86 one could even consider using an INT
instruction in order to reduce the NOP footprint in the unarmed case. Nor is
__fentry__ a C function; it has far more of an exception-like ABI.
*Regardless* of what else we do, I believe __fentry__ ought to
save/disable/restore AC, just like an exception does.
That does make sense to me. However it doesn't solve the issue of
calling (or preventing to call) some function that rescheds.
The idea of using s/a gcc plugin/objtool/ for this isn't really a bad idea.
Obviously the general problem is undecidable :) but the enforcement of some
simple, fairly draconian rules ("as tight as possible, but no tighter")
shouldn't be a huge problem.
An actual gcc plugin -- which would probably be quite complex -- could make
gcc itself aware of user space accesses and be able to rearrange them to
minimize STAC/CLAC and avoid kernel-space accesses inside those brackets.
Not sure I get this. The data that is retrieved from/stored user space
is generally obtained from/saved into kernel-space address. And when you
start the user_access_begin() it means you plan on doing a bunch of user
access, so I wouldn't expect to be able to batch everything into registers.
Cheers,
--
Julien Thierry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <hidden> Date: 2019-02-21 12:46:40
On Wed, Feb 20, 2019 at 02:55:59PM -0800, H. Peter Anvin wrote:
On 2/19/19 4:48 AM, Will Deacon wrote:
quoted
I think you'll still hate this, but could we not disable preemption during
the uaccess-enabled region, re-enabling it on the fault path after we've
toggled uaccess off and disable it again when we return back to the
uaccess-enabled region? Doesn't help with tracing, but it should at least
handle the common case.
There is a worse problem with this, I still realize: this would mean blocking
preemption across what could possibly be a *very* large copy_from_user(), for
example.
I don't think it's legitimate to call copy_{to,from}_user() inside a
user_access_{begin,end} region. You'd need to add some unsafe variants,
which could periodically disable uaccess and call cond_resched() inside
the loop to avoid the problem you're eluding to.
For existing callers of copy_{to,from}_user(), there's no issue as they
don't call into the scheduler during the copy operation. Exceptions are
handled fine by the code in mainline today.
GCC plugins are a cool idea, but I'm just a bit nervous about relying on
them for things like this.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Thomas Gleixner <hidden> Date: 2019-02-21 21:35:58
On Thu, 21 Feb 2019, Julien Thierry wrote:
On 20/02/2019 22:55, H. Peter Anvin wrote:
quoted
Now, __fentry__ is kind of a special beast; in some ways it is an "exception
implemented as a function call"; on x86 one could even consider using an INT
instruction in order to reduce the NOP footprint in the unarmed case. Nor is
__fentry__ a C function; it has far more of an exception-like ABI.
*Regardless* of what else we do, I believe __fentry__ ought to
save/disable/restore AC, just like an exception does.
That does make sense to me. However it doesn't solve the issue of
calling (or preventing to call) some function that rescheds.
IMNSHO any call inside a AC region is a bug lurking round the corner. The
only thing which is tolerable is an exception of some sort.
Enforce that with objtool. End of story.
Thanks,
tglx
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Lutomirski <luto@amacapital.net> Date: 2019-02-21 22:06:32
On Feb 21, 2019, at 4:46 AM, Will Deacon [off-list ref] wrote:
quoted
On Wed, Feb 20, 2019 at 02:55:59PM -0800, H. Peter Anvin wrote:
quoted
On 2/19/19 4:48 AM, Will Deacon wrote:
I think you'll still hate this, but could we not disable preemption during
the uaccess-enabled region, re-enabling it on the fault path after we've
toggled uaccess off and disable it again when we return back to the
uaccess-enabled region? Doesn't help with tracing, but it should at least
handle the common case.
There is a worse problem with this, I still realize: this would mean blocking
preemption across what could possibly be a *very* large copy_from_user(), for
example.
I don't think it's legitimate to call copy_{to,from}_user() inside a
user_access_{begin,end} region. You'd need to add some unsafe variants,
which could periodically disable uaccess and call cond_resched() inside
the loop to avoid the problem you're eluding to.
On Thu, Feb 21, 2019 at 1:35 PM Thomas Gleixner [off-list ref] wrote:
IMNSHO any call inside a AC region is a bug lurking round the corner. The
only thing which is tolerable is an exception of some sort.
Enforce that with objtool. End of story.
Not quite that simple.
There are some use cases where you really do want a call - albeit to
special functions - inside the AC region.
The prime example of this is likely "filldir()" in fs/readdir.c, which
is actually somewhat important for some loads (eg samba).
And the whole AC thing made it horribly horribly slow because readdir
is one of those things that doesn't just copy one value (or one
structure) to user space, but writes several different things.
Kind of like signal frame setup does.
You may not realize that right now signal frame setup *does* actually
do a call with AC set. See ia32_setup_rt_frame() that does
put_user_try {
...
compat_save_altstack_ex(&frame->uc.uc_stack, regs->sp);
...
} put_user_catch(err);
is a macro, but inside that macro is a call to sas_ss_reset().
Now, that is an inline function, so it hopefully doesn't actually
cause a call. But it's "only" an inline function, not "always_inline".
We did already have (and I rejected, for now) patches that made those
inlines not very strong...
[ Side note: currently signal frame setup uses the absolutely
*disgusting* put_user_ex() thing, but that's actually what
unsafe_put_user() was designed for. I just never got around to getting
rid of the nasty hot mess of put_user_ex() ]
Anyway, while signal frame setup just writes individual values,
"filldir()" does *both* several individual values _and_ does a
copy_to_user().
And it's that "copy_to_user()" that I want to eventually change to a
"unsafe_copy_to_user()", along with making the individual values be
written with unsafe_put_user().
Right now "filldir()" messes with AC a total of *six* times. It sets
four field values, and then does a "copy_to_user()", all of which
set/clear AC right now. It's wasting hundreds of cycles on this,
because AC is so slow to set/clear.
If AC was cheap, this wouldn't be much of an issue. But AC is really
really expensive. I think it's microcode and serializes the pipeline
or something.
Anyway. We already have a possible call site, and there are good
reasons for future ones too. But they are hopefully all very
controlled.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-22 12:58:41
On Thu, Feb 21, 2019 at 02:08:11PM -0800, Linus Torvalds wrote:
On Thu, Feb 21, 2019 at 1:35 PM Thomas Gleixner [off-list ref] wrote:
quoted
IMNSHO any call inside a AC region is a bug lurking round the corner. The
only thing which is tolerable is an exception of some sort.
Enforce that with objtool. End of story.
Not quite that simple.
There are some use cases where you really do want a call - albeit to
special functions - inside the AC region.
The prime example of this is likely "filldir()" in fs/readdir.c, which
is actually somewhat important for some loads (eg samba).
And the whole AC thing made it horribly horribly slow because readdir
is one of those things that doesn't just copy one value (or one
structure) to user space, but writes several different things.
Kind of like signal frame setup does.
You may not realize that right now signal frame setup *does* actually
do a call with AC set. See ia32_setup_rt_frame() that does
put_user_try {
...
compat_save_altstack_ex(&frame->uc.uc_stack, regs->sp);
...
} put_user_catch(err);
is a macro, but inside that macro is a call to sas_ss_reset().
Now, that is an inline function, so it hopefully doesn't actually
cause a call. But it's "only" an inline function, not "always_inline".
We did already have (and I rejected, for now) patches that made those
inlines not very strong...
That one does indeed not generate a call, but:
arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x38: call to native_load_gs_index() with AC set
I've yet to look at detectoring __preempt_count mucking.
---
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \-I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi\-I$(srctree)/tools/objtool/arch/$(ARCH)/includeWARNINGS:=$(EXTRA_WARNINGS)-Wno-switch-default-Wno-switch-enum-Wno-packed-CFLAGS+=-Werror$(WARNINGS)$(KBUILD_HOSTCFLAGS)-g$(INCLUDES)+CFLAGS+=-Werror$(WARNINGS)$(KBUILD_HOSTCFLAGS)-g$(INCLUDES)-ggdb3LDFLAGS+=-lelf$(LIBSUBCMD)$(KBUILD_HOSTLDFLAGS)# Allow old libelf to be used:
@@ -1897,11 +1917,16 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,if(ret)return1;}-}+}elseprintf("ponies\n");switch(insn->type){caseINSN_RETURN:+if(state.ac){+WARN_FUNC("return with AC set",sec,insn->offset);+return1;+}+if(func&&has_modified_stack_frame(&state)){WARN_FUNC("return with modified stack frame",sec,insn->offset);
@@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,return0;caseINSN_CALL:+if(state.ac&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(is_fentry_call(insn))break;
@@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,/* fallthrough */caseINSN_CALL_DYNAMIC:+if(state.ac&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}if(!no_fp&&func&&!has_valid_stack_frame(&state)){WARN_FUNC("call without frame pointer save/setup",sec,insn->offset);
@@ -166,8 +166,8 @@ int special_get_alts(struct elf *elf, struct list_head *alts)continue;if(sec->len%entry->size!=0){-WARN("%s size not a multiple of %d",-sec->name,entry->size);+WARN("%s size (%d) not a multiple of %d",+sec->name,sec->len,entry->size);return-1;}
From: Thomas Gleixner <hidden> Date: 2019-02-22 18:10:44
On Thu, 21 Feb 2019, Linus Torvalds wrote:
On Thu, Feb 21, 2019 at 1:35 PM Thomas Gleixner [off-list ref] wrote:
quoted
IMNSHO any call inside a AC region is a bug lurking round the corner. The
only thing which is tolerable is an exception of some sort.
Enforce that with objtool. End of story.
Not quite that simple.
But correct :)
Right now "filldir()" messes with AC a total of *six* times. It sets
four field values, and then does a "copy_to_user()", all of which
set/clear AC right now. It's wasting hundreds of cycles on this,
because AC is so slow to set/clear.
If AC was cheap, this wouldn't be much of an issue. But AC is really
really expensive. I think it's microcode and serializes the pipeline
or something.
Anyway. We already have a possible call site, and there are good
reasons for future ones too. But they are hopefully all very
controlled.
I agree, that a function which is doing the actual copy should be callable,
but random other functions? NO!
The problem is that once you open that can of worms the people who think
their problem is special will come around and do
begin()
copy_to_user_unsafe(uptr, collect_data())
end()
just because they can. That's the stuff, I'm worried about, not the well
defined copy_to/from_user() invocation. We can deal with that and make sure
that it's safe even with tracing and annotate with some special magic. It's
simpler to find and check a new '__safe_inside_ac' annotation than chasing
randomly added code within a gazillion of begin/end sections.
Thanks,
tglx
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-22 22:26:48
On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
But correct :)
I agree, that a function which is doing the actual copy should be callable,
but random other functions? NO!
So find the below patch -- which spotted fail in ptrace.c
It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':
arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set
It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
lack of notrace annotations on functions marked AC_SAFE():
arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set
It builds arch/x86 relatively clean; it only complains about some
redundant CLACs in entry_64.S because it doesn't understand interrupts
and I've not bothered creating an annotation for them yet.
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC
...
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
Also, I realized we don't need special annotations for preempt_count;
preempt_disable() emits a CALL instruction which should readily trigger
the warnings added here.
The VDSO thing is a bit of a hack, but I couldn't quickly find anything
better.
Comments?
---
arch/x86/include/asm/special_insns.h | 2 ++
arch/x86/kernel/ptrace.c | 3 +-
include/linux/frame.h | 23 ++++++++++++++
tools/objtool/arch.h | 4 ++-
tools/objtool/arch/x86/decode.c | 14 ++++++++-
tools/objtool/check.c | 59 ++++++++++++++++++++++++++++++++++++
tools/objtool/check.h | 3 +-
tools/objtool/elf.h | 1 +
8 files changed, 105 insertions(+), 4 deletions(-)
@@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,switch(insn->type){caseINSN_RETURN:+if(state.ac){+WARN_FUNC("return with AC set",sec,insn->offset);+return1;+}+if(func&&has_modified_stack_frame(&state)){WARN_FUNC("return with modified stack frame",sec,insn->offset);
@@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,return0;caseINSN_CALL:+if((state.ac_safe||state.ac)&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(is_fentry_call(insn))break;
@@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,/* fallthrough */caseINSN_CALL_DYNAMIC:+if((state.ac_safe||state.ac)&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}if(!no_fp&&func&&!has_valid_stack_frame(&state)){WARN_FUNC("call without frame pointer save/setup",sec,insn->offset);
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra [off-list ref] wrote:
So find the below patch -- which spotted fail in ptrace.c
It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':
Looks sane enough to me.
Can you make it do DF too while at it? I really think AC and DF are
the same in this context. If you call an arbitrary function with DF
set, things will very quickly go sideways (even if it might work in
practice as long as the function just doesn't do a memcpy or similar)
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On February 22, 2019 2:26:35 PM PST, Peter Zijlstra [off-list ref] wrote:
quoted hunk
On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
quoted
But correct :)
quoted
I agree, that a function which is doing the actual copy should be
callable,
quoted
but random other functions? NO!
So find the below patch -- which spotted fail in ptrace.c
It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':
arch/x86/ia32/ia32_signal.o: warning: objtool:
ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC
set
arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to
getreg() with AC set
It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
lack of notrace annotations on functions marked AC_SAFE():
arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to
__fentry__() with AC set
It builds arch/x86 relatively clean; it only complains about some
redundant CLACs in entry_64.S because it doesn't understand interrupts
and I've not bothered creating an annotation for them yet.
arch/x86/entry/entry_64.o: warning: objtool:
.altinstr_replacement+0x4d: redundant CLAC
arch/x86/entry/entry_64.o: warning: objtool:
.altinstr_replacement+0x5a: redundant CLAC
...
arch/x86/entry/entry_64.o: warning: objtool:
.altinstr_replacement+0xb1: redundant CLAC
Also, I realized we don't need special annotations for preempt_count;
preempt_disable() emits a CALL instruction which should readily trigger
the warnings added here.
The VDSO thing is a bit of a hack, but I couldn't quickly find anything
better.
Comments?
---
arch/x86/include/asm/special_insns.h | 2 ++
arch/x86/kernel/ptrace.c | 3 +-
include/linux/frame.h | 23 ++++++++++++++
tools/objtool/arch.h | 4 ++-
tools/objtool/arch/x86/decode.c | 14 ++++++++-
tools/objtool/check.c | 59
++++++++++++++++++++++++++++++++++++
tools/objtool/check.h | 3 +-
tools/objtool/elf.h | 1 +
8 files changed, 105 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/special_insns.h
b/arch/x86/include/asm/special_insns.h
index 43c029cdc3fe..cd31e4433f4c 100644
#endif /* CONFIG_STACK_VALIDATION */
+#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) &&
!defined(BUILD_VDSO32)
+/*
+ * This macro marks functions as AC-safe, that is, it is safe to call
from an
+ * EFLAGS.AC enabled region (typically user_access_begin() /
+ * user_access_end()).
+ *
+ * These functions in turn will only call AC-safe functions themselves
(which
+ * precludes tracing, including __fentry__ and scheduling, including
+ * preempt_enable).
+ *
+ * AC-safe functions will obviously also not change EFLAGS.AC
themselves.
+ *
+ * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO
builds
+ * (and the generated symbol reference will in fact cause link
failures).
+ */
+#define AC_SAFE(func) \
+ static void __used __section(.discard.ac_safe) \
+ *__func_ac_safe_##func = func
+
+#else
+#define AC_SAFE(func)
+#endif
+
#endif /* _LINUX_FRAME_H */
unsigned long offset;
unsigned int len;
struct symbol *pfunc, *cfunc;
+ bool ac_safe;
};
struct rela {
I like it.
Objtool could also detect CLAC-STAC or STAC-CLAC sequences without memory operations and remove them; don't know how often that happens, but I know it *does* happen.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Lutomirski <luto@kernel.org> Date: 2019-02-22 23:55:42
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra [off-list ref] wrote:
On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
quoted
But correct :)
quoted
I agree, that a function which is doing the actual copy should be callable,
but random other functions? NO!
So find the below patch -- which spotted fail in ptrace.c
It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':
arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set
It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
lack of notrace annotations on functions marked AC_SAFE():
arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set
It builds arch/x86 relatively clean; it only complains about some
redundant CLACs in entry_64.S because it doesn't understand interrupts
and I've not bothered creating an annotation for them yet.
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC
...
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
Does objtool understand altinstr? If it understands that this is an
altinstr associated with a not-actually-a-fuction (i.e. END not
ENDPROC) piece of code, it could suppress this warning.
quoted hunk
-static unsigned long getreg(struct task_struct *task, unsigned long offset)+static notrace unsigned long getreg(struct task_struct *task, unsigned long offset) { switch (offset) { case offsetof(struct user_regs_struct, cs):
@@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset) return *pt_regs_access(task_pt_regs(task), offset); }+AC_SAFE(getreg);
This takes the address and could plausibly suppress some optimizations.
quoted hunk
+#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32)+/*+ * This macro marks functions as AC-safe, that is, it is safe to call from an+ * EFLAGS.AC enabled region (typically user_access_begin() /+ * user_access_end()).+ *+ * These functions in turn will only call AC-safe functions themselves (which+ * precludes tracing, including __fentry__ and scheduling, including+ * preempt_enable).+ *+ * AC-safe functions will obviously also not change EFLAGS.AC themselves.+ *+ * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds+ * (and the generated symbol reference will in fact cause link failures).+ */+#define AC_SAFE(func) \+ static void __used __section(.discard.ac_safe) \+ *__func_ac_safe_##func = func
Are we okay with annotating function *declarations* in a way that
their addresses get taken whenever the declaration is processed? It
would be nice if we could avoid this.
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
--Andy
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Lutomirski <luto@kernel.org> Date: 2019-02-23 00:35:02
[mailing lists removed because this is a potentially large source of exploits]
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra [off-list ref] wrote:
On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
quoted
But correct :)
quoted
I agree, that a function which is doing the actual copy should be callable,
but random other functions? NO!
So find the below patch -- which spotted fail in ptrace.c
Um, wait a moment. You didn't find an oddity in ptrace.c. You found
a giant freaking error in uaccess.h.
Am I missing something? How are there not zillions of instances of
this that your patch ought to catch? Or is genregs_get() really the
only example?
On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski [off-list ref] wrote:
[mailing lists removed because this is a potentially large source of exploits]
I think you're overly worried.
AC doesn't protect against "large source of exploits". If it did, then
all CPU's before Broadwell would be insecure. They aren't. They'd
better not be, considering that there's a _lot_ of Xeon machines out
there based on older microarchitectures. I think some of them might
even be reasonably current (eg Xeon E7v3 isn't _that_ old, and is
Haswell-based, and doesn't have SMAP afaik).
SMAP is a great debugging and development aid, and makes sure that
developers - who hopefully run primarily on modern platforms - don't
write code that just accesses user space directly (because with SMAP,
it won't work).
And yes, SMAP can limit the effect of kernel bugs, and turn something
that would otherwise be a security issue into "just a bug".
But running with AC on isn't a security issue in itself - it just
makes SMAP slightly less powerful. The biggest issue of the whole "AC
doesn't get saved/redstored" is actually the *reverse* case, where a
preemption event could then cause a process that had AC on to be
scheduled away, then AC would stay on for some time, but then we might
schedule back with AC _clear_, and now you'd have a non-working user
access, and a possible DoS attack as a result because you returned
EFAULT to a system call that was perfectly fine.
See? It's not so much "AC stays on" that is a "sky is falling" issue,
it's actually "AC also gets turned off randomly" that actually has
real and immediate effects. "AC on" is unfortunate and not great,
don't get me wrong, but it's definitely not the end of the world.
Particularly not for short sequences.
That said:
Um, wait a moment. You didn't find an oddity in ptrace.c. You found
a giant freaking error in uaccess.h.
I agree that your patch is good, and should be applied. Mind writing
up a changelog and committing it to -tip?
Am I missing something? How are there not zillions of instances of
this that your patch ought to catch? Or is genregs_get() really the
only example?
I really do think that it's very unusual to do "get/put_user()" with
complicated value arguments. So while your patch is obviously the
right thing to do, I really don't think this is a huge worry, or all
_that_ surprising that this issue apparently found just a single case
of a function call.
With all that said, I didn't even react to this part of PeterZ's
patch, but it's a good call, and I think it's also a great validation
of the objtool approach to validating AC. So cheers for that!
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Lutomirski <luto@kernel.org> Date: 2019-02-23 01:17:20
On Fri, Feb 22, 2019 at 5:12 PM Linus Torvalds
[off-list ref] wrote:
On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski [off-list ref] wrote:
quoted
[mailing lists removed because this is a potentially large source of exploits]
I think you're overly worried.
AC doesn't protect against "large source of exploits".
What I meant was: a potentially large grab bag of ways to help turn a
bug into an exploit.
That said:
quoted
Um, wait a moment. You didn't find an oddity in ptrace.c. You found
a giant freaking error in uaccess.h.
I agree that your patch is good, and should be applied. Mind writing
up a changelog and committing it to -tip?
I don't have commit access :) But I shall email it out once I test it a bit.
--Andy
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Feb 22, 2019 at 5:17 PM Andy Lutomirski [off-list ref] wrote:
What I meant was: a potentially large grab bag of ways to help turn a
bug into an exploit.
Well, apparently there's only one place, and that one looks pretty harmless.
Of course, I don't think PeterZ said what his config was. Maybe it was
a very minimal config and only found that one case because all the
truly scary cases were configured out ;)
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Feb 22, 2019 at 5:17 PM Andy Lutomirski [off-list ref] wrote:
I don't have commit access :) But I shall email it out once I test it a bit.
Btw, looking at it, with that change every user of __put_user_size()
now passes in the properly type-cast value in 'x'.
Which means that you could remove the odd typecast in the 8-byte case:
case 8: \
__put_user_goto_u64((__typeof__(*ptr))(x), ptr, label); \
and make it match all the other cases:
case 8: \
__put_user_goto_u64(x, ptr, label); \
but that's just from looking at the patch, and maybe I'm missing something.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Yep, otherwise it would've never found any of the STAC/CLAC instructions
to begin with, they're all alternatives. The emitted code is all NOPs.
If it understands that this is an
altinstr associated with a not-actually-a-fuction (i.e. END not
ENDPROC) piece of code, it could suppress this warning.
Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for
these 'functions', so yes, I can try and ignore this warning for those.
Are we okay with annotating function *declarations* in a way that
their addresses get taken whenever the declaration is processed? It
would be nice if we could avoid this.
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
Fixing that code would be fine; but we need that annotation regardless,
read Linus' email a little further back, he wants things like
copy_{to,from}_user_unsafe().
Those really would need to be marked AC-safe, there's no inlining that.
That said, yes these annotations _suck_. But it's what we had and works,
I'm just copying it (from STACK_FRAME_NON_STANDARD).
That said; maybe we can do something like:
#define AC_SAFE(func) \
asm (".pushsection .discard.ac_safe_sym\n\t" \
"999: .ascii \"" #func "\"\n\t" \
".popsection\n\t" \
".pushsection .discard.ac_safe\n\t" \
_ASM_PTR " 999b\n\t" \
".popsection")
I just don't have a clue on how to read that in objtool; weak elf foo.
I'll see if I can make it work.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-23 08:39:51
On Fri, Feb 22, 2019 at 03:39:48PM -0800, hpa@zytor.com wrote:
Objtool could also detect CLAC-STAC or STAC-CLAC sequences without
memory operations and remove them; don't know how often that happens,
but I know it *does* happen.
Objtool doesn't know about memops; that'd be a lot of work. Also,
objtool doesn't actually rewrite the text, at best it could warn about
such occurences.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Yep, otherwise it would've never found any of the STAC/CLAC instructions
to begin with, they're all alternatives. The emitted code is all NOPs.
quoted
If it understands that this is an
altinstr associated with a not-actually-a-fuction (i.e. END not
ENDPROC) piece of code, it could suppress this warning.
Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for
these 'functions', so yes, I can try and ignore this warning for those.
Are we okay with annotating function *declarations* in a way that
their addresses get taken whenever the declaration is processed? It
would be nice if we could avoid this.
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
Fixing that code would be fine; but we need that annotation regardless,
read Linus' email a little further back, he wants things like
copy_{to,from}_user_unsafe().
Those really would need to be marked AC-safe, there's no inlining that.
That said, yes these annotations _suck_. But it's what we had and works,
I'm just copying it (from STACK_FRAME_NON_STANDARD).
That said; maybe we can do something like:
#define AC_SAFE(func) \
asm (".pushsection .discard.ac_safe_sym\n\t" \
"999: .ascii \"" #func "\"\n\t" \
".popsection\n\t" \
".pushsection .discard.ac_safe\n\t" \
_ASM_PTR " 999b\n\t" \
".popsection")
I just don't have a clue on how to read that in objtool; weak elf foo.
I'll see if I can make it work.
Fixed all that. Should probably also convert that NON_STANDARD stuff to
this new shiny thing.
Retained the ptrace muck because it's a nice test case, your patch is
obviously a better fix there.
Haven't bothered looking at alternatives yet, this is a
defconfig+tracing build.
Weekend now.
---
arch/x86/include/asm/special_insns.h | 2 +
arch/x86/kernel/ptrace.c | 3 +-
include/linux/frame.h | 38 ++++++++++++
tools/objtool/Makefile | 2 +-
tools/objtool/arch.h | 6 +-
tools/objtool/arch/x86/decode.c | 22 ++++++-
tools/objtool/check.c | 117 ++++++++++++++++++++++++++++++++++-
tools/objtool/check.h | 2 +-
tools/objtool/elf.h | 1 +
9 files changed, 187 insertions(+), 6 deletions(-)
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \-I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi\-I$(srctree)/tools/objtool/arch/$(ARCH)/includeWARNINGS:=$(EXTRA_WARNINGS)-Wno-switch-default-Wno-switch-enum-Wno-packed-CFLAGS+=-Werror$(WARNINGS)$(KBUILD_HOSTCFLAGS)-g$(INCLUDES)+CFLAGS+=-Werror$(WARNINGS)$(KBUILD_HOSTCFLAGS)-g$(INCLUDES)-ggdb3LDFLAGS+=-lelf$(LIBSUBCMD)$(KBUILD_HOSTLDFLAGS)# Allow old libelf to be used:
@@ -451,6 +451,41 @@ static void add_ignores(struct objtool_file *file)}}+staticintadd_ac_safe(structobjtool_file*file)+{+structsection*sec_sym,*sec;+structsymbol*func;+structrela*rela;+constchar*name;++sec=find_section_by_name(file->elf,".rela.discard.ac_safe");+if(!sec)+return0;++sec_sym=find_section_by_name(file->elf,".discard.ac_safe_sym");+if(!sec_sym){+WARN("missing AC-safe symbols");+return-1;+}++list_for_each_entry(rela,&sec->rela_list,list){+if(rela->sym->type!=STT_SECTION){+WARN("unexpected relocation symbol type in %s",sec->name);+return-1;+}++name=elf_strptr(file->elf->elf,sec_sym->idx,rela->addend);++func=find_symbol_by_name(file->elf,name);+if(!func)+continue;++func->ac_safe=true;+}++return0;+}+/**FIXME:Fornow,justignoreanyalternativeswhichaddretpolines.Thisis*atemporaryhack,asitdoesn'tallowORCtounwindfrominsidearetpoline.
@@ -695,6 +730,7 @@ static int handle_group_alt(struct objtool_file *file,last_new_insn=insn;insn->ignore=orig_insn->ignore_alts;+insn->func=orig_insn->func;if(insn->type!=INSN_JUMP_CONDITIONAL&&insn->type!=INSN_JUMP_UNCONDITIONAL)
@@ -1239,6 +1275,10 @@ static int decode_sections(struct objtool_file *file)add_ignores(file);+ret=add_ac_safe(file);+if(ret)+returnret;+ret=add_nospec_ignores(file);if(ret)returnret;
@@ -1902,6 +1942,15 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,switch(insn->type){caseINSN_RETURN:+if(state.ac){+WARN_FUNC("return with AC set",sec,insn->offset);+return1;+}+if(state.df){+WARN_FUNC("return with DF set",sec,insn->offset);+return1;+}+if(func&&has_modified_stack_frame(&state)){WARN_FUNC("return with modified stack frame",sec,insn->offset);
@@ -1917,6 +1966,17 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,return0;caseINSN_CALL:+if((state.ac_safe||state.ac)&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(state.df){+WARN_FUNC("call to %s() with DF set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(is_fentry_call(insn))break;
@@ -1926,8 +1986,25 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,if(ret==-1)return1;-/* fallthrough */+if(!no_fp&&func&&!has_valid_stack_frame(&state)){+WARN_FUNC("call without frame pointer save/setup",+sec,insn->offset);+return1;+}+break;+caseINSN_CALL_DYNAMIC:+if((state.ac_safe||state.ac)&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(state.df){+WARN_FUNC("call to %s() with DF set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(!no_fp&&func&&!has_valid_stack_frame(&state)){WARN_FUNC("call without frame pointer save/setup",sec,insn->offset);
On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
quoted
But correct :)
quoted
I agree, that a function which is doing the actual copy should be callable,
but random other functions? NO!
So find the below patch -- which spotted fail in ptrace.c
It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':
arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set
It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
lack of notrace annotations on functions marked AC_SAFE():
arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set
It builds arch/x86 relatively clean; it only complains about some
redundant CLACs in entry_64.S because it doesn't understand interrupts
and I've not bothered creating an annotation for them yet.
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC
...
arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
Also, I realized we don't need special annotations for preempt_count;
preempt_disable() emits a CALL instruction which should readily trigger
the warnings added here.
The VDSO thing is a bit of a hack, but I couldn't quickly find anything
better.
Comments?
I haven't looked at all the details. But could the annotation be called
UACCESS_SAFE() (and corresponding naming in the objtool checks)? Since
this is not an x86 only issue and the AC flags only exists for x86.
Cheers,
Julien
@@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,switch(insn->type){caseINSN_RETURN:+if(state.ac){+WARN_FUNC("return with AC set",sec,insn->offset);+return1;+}+if(func&&has_modified_stack_frame(&state)){WARN_FUNC("return with modified stack frame",sec,insn->offset);
@@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,return0;caseINSN_CALL:+if((state.ac_safe||state.ac)&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}+if(is_fentry_call(insn))break;
@@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,/* fallthrough */caseINSN_CALL_DYNAMIC:+if((state.ac_safe||state.ac)&&!insn->call_dest->ac_safe){+WARN_FUNC("call to %s() with AC set",sec,insn->offset,+insn->call_dest->name);+return1;+}if(!no_fp&&func&&!has_valid_stack_frame(&state)){WARN_FUNC("call without frame pointer save/setup",sec,insn->offset);
On February 23, 2019 12:39:42 AM PST, Peter Zijlstra [off-list ref] wrote:
On Fri, Feb 22, 2019 at 03:39:48PM -0800, hpa@zytor.com wrote:
quoted
Objtool could also detect CLAC-STAC or STAC-CLAC sequences without
memory operations and remove them; don't know how often that happens,
but I know it *does* happen.
Objtool doesn't know about memops; that'd be a lot of work. Also,
objtool doesn't actually rewrite the text, at best it could warn about
such occurences.
It doesn't have to understand the contents of the memop, but it seems that the presence of a modrm with mode ≠ 3 should be plenty. It needs to know that much in order to know the length of instructions anyway. For extra credit, ignore LEA or hinting instructions.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On February 23, 2019 12:39:42 AM PST, Peter Zijlstra [off-list ref] wrote:
On Fri, Feb 22, 2019 at 03:39:48PM -0800, hpa@zytor.com wrote:
quoted
Objtool could also detect CLAC-STAC or STAC-CLAC sequences without
memory operations and remove them; don't know how often that happens,
but I know it *does* happen.
Objtool doesn't know about memops; that'd be a lot of work. Also,
objtool doesn't actually rewrite the text, at best it could warn about
such occurences.
It doesn't even have to change the text per se, just nullify the altinstr.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-25 10:52:09
On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
Paravirt can go bugger off. There's no sane way to fix that.
Luckily the load_gs_index() thing is part of the paravirt me harder crap
and so nobody sane should ever hit that.
I don't fully understand that code at all; I also have no clue why GS
has paravirt bits on but the other segments do not. But it looks like we
want to do that RELOAD_SEG() crap under SMAP because of the GET_SEG() ->
get_user_ex() thing.
Anyway, I only see 3 options here:
1) live with the paravirt me harder builds complaining
2) exclude the AC validation from the paravirt me harder builds
3) rewrite this code to no need that stupid call in the first place
2 seems like an exceptionally bad ideal, 3 would need someone that
understands this, so for now I'll pick 1 :-)
*thought*... we could delay the actual set_user_seg() thing until after
the get_user_catch(), would that work?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-25 11:54:05
On Mon, Feb 25, 2019 at 11:51:44AM +0100, Peter Zijlstra wrote:
On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
quoted
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
Paravirt can go bugger off. There's no sane way to fix that.
I don't fully understand that code at all; I also have no clue why GS
has paravirt bits on but the other segments do not.
*sigh* SWAPGS
*thought*... we could delay the actual set_user_seg() thing until after
the get_user_catch(), would that work?
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-25 11:55:31
On Mon, Feb 25, 2019 at 08:33:35AM +0000, Julien Thierry wrote:
quoted
It has an AC_SAFE(func) annotation which allows marking specific
functions as safe to call. The patch includes 2 instances which were
required to make arch/x86 'build':
I haven't looked at all the details. But could the annotation be called
UACCESS_SAFE() (and corresponding naming in the objtool checks)? Since
this is not an x86 only issue and the AC flags only exists for x86.
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-02-25 13:21:17
On Mon, Feb 25, 2019 at 12:47:00AM -0800, hpa@zytor.com wrote:
It doesn't have to understand the contents of the memop, but it seems
that the presence of a modrm with mode ≠ 3 should be plenty. It needs
to know that much in order to know the length of instructions anyway.
For extra credit, ignore LEA or hinting instructions.
A little something like so then?
arch/x86/kernel/fpu/signal.o: warning: objtool: .altinstr_replacement+0x9c: UACCESS disable without MEMOPs: copy_fpstate_to_sigframe()
00000000023c 000200000002 R_X86_64_PC32 0000000000000000 .text + 604
000000000240 000700000002 R_X86_64_PC32 0000000000000000 .altinstr_replacement + 99
000000000249 000200000002 R_X86_64_PC32 0000000000000000 .text + 610
00000000024d 000700000002 R_X86_64_PC32 0000000000000000 .altinstr_replacement + 9c
.text
604: 90 nop
605: 90 nop
606: 90 nop
607: 83 ce 03 or $0x3,%esi
60a: 89 b3 00 02 00 00 mov %esi,0x200(%rbx)
610: 90 nop
611: 90 nop
612: 90 nop
.altinstr_replacement
99: 0f 01 cb stac
9c: 0f 01 ca clac
Which looks like the tool failed to recognise that MOV as a memop.
---
From: Andy Lutomirski <luto@amacapital.net> Date: 2019-02-25 15:36:28
quoted hunk
On Feb 25, 2019, at 3:53 AM, Peter Zijlstra [off-list ref] wrote:
quoted
On Mon, Feb 25, 2019 at 11:51:44AM +0100, Peter Zijlstra wrote:
quoted
On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
Paravirt can go bugger off. There's no sane way to fix that.
quoted
I don't fully understand that code at all; I also have no clue why GS
has paravirt bits on but the other segments do not.
*sigh* SWAPGS
quoted
*thought*... we could delay the actual set_user_seg() thing until after
the get_user_catch(), would that work?
@@ -85,10 +89,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, * the handler, but does not clobber them at least in the * normal case. */- RELOAD_SEG(gs);- RELOAD_SEG(fs);- RELOAD_SEG(ds);- RELOAD_SEG(es);+ LOAD_SEG(gs);+ LOAD_SEG(fs);+ LOAD_SEG(ds);+ LOAD_SEG(es); COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx); COPY(dx); COPY(cx); COPY(ip); COPY(ax);
I would call this pretty horrible. How about we do it without macros? :)
But yes, deferring the segment load until after the read seems fine to me. Frankly, we could also just copy_from_user the whole thing up front — thus code is not really a serious fast path.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2019-03-01 15:08:01
On Mon, Feb 25, 2019 at 02:21:03PM +0100, Peter Zijlstra wrote:
On Mon, Feb 25, 2019 at 12:47:00AM -0800, hpa@zytor.com wrote:
quoted
It doesn't have to understand the contents of the memop, but it seems
that the presence of a modrm with mode ≠ 3 should be plenty. It needs
to know that much in order to know the length of instructions anyway.
For extra credit, ignore LEA or hinting instructions.
A little something like so then?
$ ./objtool check --no-fp --backtrace ../../defconfig-build/arch/x86/lib/usercopy_64.o
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x3: UACCESS disable without MEMOPs: __clear_user()
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x3a: (alt)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x2e: (branch)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x18: (branch)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0xffffffffffffffff: (branch)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x5: (alt)
../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x0: <=== (func)
0000000000000000 <__clear_user>:
0: e8 00 00 00 00 callq 5 <__clear_user+0x5>
1: R_X86_64_PLT32 __fentry__-0x4
5: 90 nop
6: 90 nop
7: 90 nop
8: 48 89 f0 mov %rsi,%rax
b: 48 c1 ee 03 shr $0x3,%rsi
f: 83 e0 07 and $0x7,%eax
12: 48 89 f1 mov %rsi,%rcx
15: 48 85 c9 test %rcx,%rcx
18: 74 0f je 29 <__clear_user+0x29>
1a: 48 c7 07 00 00 00 00 movq $0x0,(%rdi)
21: 48 83 c7 08 add $0x8,%rdi
25: ff c9 dec %ecx
27: 75 f1 jne 1a <__clear_user+0x1a>
29: 48 89 c1 mov %rax,%rcx
2c: 85 c9 test %ecx,%ecx
2e: 74 0a je 3a <__clear_user+0x3a>
30: c6 07 00 movb $0x0,(%rdi)
33: 48 ff c7 inc %rdi
36: ff c9 dec %ecx
38: 75 f6 jne 30 <__clear_user+0x30>
3a: 90 nop
3b: 90 nop
3c: 90 nop
3d: 48 89 c8 mov %rcx,%rax
40: c3 retq
Seems correct. Not sure you want to go fix that though. Let me know if
you want more output.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel