Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1695516 > unrolled thread

Re: [PATCH v3 00/10] x86: ORC unwinder (previously undwarf)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 00/10] x86: ORC unwinder (previously undwarf) Ingo Molnar <mingo@kernel.org> - 2017-07-25 11:10 +0200
    Re: [PATCH v3 00/10] x86: ORC unwinder (previously undwarf) Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-25 20:00 +0200
      Re: [PATCH v3 00/10] x86: ORC unwinder (previously undwarf) Kees Cook <keescook@chromium.org> - 2017-07-25 20:50 +0200

#1695516 — Re: [PATCH v3 00/10] x86: ORC unwinder (previously undwarf)

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Jul 12, 2017 at 09:27:50PM +0200, Ingo Molnar wrote:
> > Maybe we could offer a menu of unwinders - i.e. make the whole Kconfig interface a 
> > bit nicer:
> > 
> >   CONFIG_UNWINDER_FRAME_POINTER
> >   CONFIG_UNWINDER_ORC
> >   CONFIG_UNWINDER_GUESS
> > 
> > ... or so?
> 
> So far I haven't been able to figure out how to make the above three
> options into a multiple choice selection, such that allnoconfig selects
> CONFIG_UNWINDER_GUESS and alldefconfig selects
> CONFIG_UNWINDER_FRAME_POINTER.

I don't think that's a problem: the scheduler preemption model Kconfig setup has 
similar behavior - allyesconfig does not enable CONFIG_PREEMPT=y.

The new x86 default will eventually be the Orc unwinder, but not initially.

> > I wouldn't mind making CONFIG_UNWINDER_ORC the new default either, due to the 
> > non-trivial speedup it offers - but maybe folks would object?
> 
> Personally I wouldn't have an objection to making ORC the default, though we 
> should probably wait to give it some burn-in time first.

Sure, that's what testing is for.

> If we *do* decide to eventually make it the default, we could flip the switch at 
> the same time we introduced the multiple-choice config and rename above.  That 
> way, users of "make oldconfig" would see the change and would be encouraged to 
> switch ORC.

I disagree, as the current Kconfig layout actively hinders the 'more testing' 
part: you can only enable Orc if you knew how to do it, and 99% of our testers 
won't bother. In practice that's a testing coverage that is close to not testing 
it at all ...

> > > > CONFIG_FRAME_POINTERS et al would be left for architectures where it has a meaning 
> > > > beyond backtrace generation. (Not sure whether there's any such architectures.)
> > > 
> > > Well, on x86, hardened usercopy relies on frame pointers, but not the
> > > unwinder.  It does the frame pointer walk manually to avoid the full
> > > unwinder overhead.  See arch_within_stack_frames().

BTW., I think this aspect of the hardened user-copy is crazy stuff - there can be 
many stack frames, and this adds a serious amount of overhead even with frame 
pointers...

I think the current behavior is fine: if frame pointers are disabled then 
arch_within_stack_frames() returns NOT_STACK. Maybe it could do a few sanity 
checks: we do know the kernel stack range and we could check alignment as well.

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1695982

[ Adding Kees to CC for the hardened usercopy discussion. ]

Kees, FYI: frame pointers may be disabled by default on x86 relatively
soon (presumably weeks or months) in favor of the ORC unwinder.  So the
hardened usercopy stack walk will no longer work as advertised.

Using the ORC unwinder for hardened usercopy would probably be pretty
bad performance-wise.  I'm not sure what else could be done.  Ingo did
have a few ideas for sanity checks:

On Tue, Jul 25, 2017 at 11:09:44AM +0200, Ingo Molnar wrote:
> > > > Well, on x86, hardened usercopy relies on frame pointers, but not the
> > > > unwinder.  It does the frame pointer walk manually to avoid the full
> > > > unwinder overhead.  See arch_within_stack_frames().
> 
> BTW., I think this aspect of the hardened user-copy is crazy stuff - there can be 
> many stack frames, and this adds a serious amount of overhead even with frame 
> pointers...
> 
> I think the current behavior is fine: if frame pointers are disabled then 
> arch_within_stack_frames() returns NOT_STACK. Maybe it could do a few sanity 
> checks: we do know the kernel stack range and we could check alignment as well.

I believe it checks the kernel stack range already in
check_stack_object() before deciding whether to call
arch_within_stack_frames().  It also has an overlapping stack check.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1696032

On Tue, Jul 25, 2017 at 10:58 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> [ Adding Kees to CC for the hardened usercopy discussion. ]
>
> Kees, FYI: frame pointers may be disabled by default on x86 relatively
> soon (presumably weeks or months) in favor of the ORC unwinder.  So the
> hardened usercopy stack walk will no longer work as advertised.
>
> Using the ORC unwinder for hardened usercopy would probably be pretty
> bad performance-wise.  I'm not sure what else could be done.  Ingo did
> have a few ideas for sanity checks:
>
> On Tue, Jul 25, 2017 at 11:09:44AM +0200, Ingo Molnar wrote:
>> > > > Well, on x86, hardened usercopy relies on frame pointers, but not the
>> > > > unwinder.  It does the frame pointer walk manually to avoid the full
>> > > > unwinder overhead.  See arch_within_stack_frames().
>>
>> BTW., I think this aspect of the hardened user-copy is crazy stuff - there can be
>> many stack frames, and this adds a serious amount of overhead even with frame
>> pointers...
>>
>> I think the current behavior is fine: if frame pointers are disabled then
>> arch_within_stack_frames() returns NOT_STACK. Maybe it could do a few sanity
>> checks: we do know the kernel stack range and we could check alignment as well.
>
> I believe it checks the kernel stack range already in
> check_stack_object() before deciding whether to call
> arch_within_stack_frames().  It also has an overlapping stack check.

Right, pointers starting in the stack are already checked to not go
beyond the stack.

As far as dropping inter-frame overflow checking, while I'd prefer to
keep it, but its benefit in my mind is already pretty minimal since it
already doesn't protect/exclude the stack canary. And since this is a
check for a linear overflow (i.e. a contiguous access) we're mostly
protected by the existing stack canary for writes. For reads, we do
risk allowing return addresses to get exposed, though without the
frame pointer, we've got even less to expose in the first place.

So, mainly, I'm fine with this. I'm slightly sad, but it's not a huge
loss. The main benefit of usercopy hardening is the slab cache object
size protections...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web