| Message ID | 20170920141309.gbrx53xahjmyrv6c@docker (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20.09.2017 17:13, Tycho Andersen wrote: > On Wed, Sep 20, 2017 at 02:27:05PM +0300, Alexander Popov wrote: >>> +/* >>> + * Note that the way this test fails is kind of ugly; it hits the BUG() in >>> + * track_stack, but then the BUG() handler blows the stack and hits the stack >>> + * guard page. >>> + */ >> >> Yes, actually, the reason is deeper. >> >> When there are less than (THREAD_SIZE / 16) bytes left in the kernel stack, the >> BUG() in track_stack() is hit. But do_error_trap(), which handles the invalid >> opcode, has a big stack frame. So it is instrumented by the STACKLEAK gcc plugin >> and itself calls track_stack() at the beginning. Hence we have a recursive >> BUG(), which eventually hits the guard page. >> >> I banned the instrumentation of do_error_trap() in the plugin, but it didn't >> really help, since there are several other instrumented functions called during >> BUG() handling. >> >> So it seems to me that this BUG() in track_stack() is really useless and can be >> dropped. Moreover: >> - it is not a part of the PaX patch; >> - it never worked in Grsecurity kernel because of the error spotted by Tycho. >> >> What do you think about it? > > We'll only have a stack guard page in the case of vmap stack, so maybe Thanks, that's an important aspect. > we can do: > > diff --git a/fs/exec.c b/fs/exec.c > index 8333c4dce59b..8351369cd1e4 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1960,7 +1960,8 @@ void __used track_stack(void) > current->thread.lowest_stack = sp; > } > > - if (unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16))) > + if (!IS_ENABLED(CONFIG_VMAP_STACK) && > + unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16))) > BUG(); > } > EXPORT_SYMBOL(track_stack); In that case the recursive BUG() in track_stack() will happen anyway. You know, I would better make CONFIG_GCC_PLUGIN_STACKLEAK depend on CONFIG_VMAP_STACK. > Anyway, thanks for the reviews, I'll post an updated version shortly. You're welcome. Best regards, Alexander
On 21.09.2017 16:26, Alexander Popov wrote: > On 20.09.2017 17:13, Tycho Andersen wrote: >> On Wed, Sep 20, 2017 at 02:27:05PM +0300, Alexander Popov wrote: >>>> +/* >>>> + * Note that the way this test fails is kind of ugly; it hits the BUG() in >>>> + * track_stack, but then the BUG() handler blows the stack and hits the stack >>>> + * guard page. >>>> + */ >>> >>> Yes, actually, the reason is deeper. >>> >>> When there are less than (THREAD_SIZE / 16) bytes left in the kernel stack, the >>> BUG() in track_stack() is hit. But do_error_trap(), which handles the invalid >>> opcode, has a big stack frame. So it is instrumented by the STACKLEAK gcc plugin >>> and itself calls track_stack() at the beginning. Hence we have a recursive >>> BUG(), which eventually hits the guard page. >>> >>> I banned the instrumentation of do_error_trap() in the plugin, but it didn't >>> really help, since there are several other instrumented functions called during >>> BUG() handling. >>> >>> So it seems to me that this BUG() in track_stack() is really useless and can be >>> dropped. Moreover: >>> - it is not a part of the PaX patch; >>> - it never worked in Grsecurity kernel because of the error spotted by Tycho. >>> >>> What do you think about it? >> >> We'll only have a stack guard page in the case of vmap stack, so maybe > > Thanks, that's an important aspect. > >> we can do: >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 8333c4dce59b..8351369cd1e4 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1960,7 +1960,8 @@ void __used track_stack(void) >> current->thread.lowest_stack = sp; >> } >> >> - if (unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16))) >> + if (!IS_ENABLED(CONFIG_VMAP_STACK) && >> + unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16))) >> BUG(); >> } >> EXPORT_SYMBOL(track_stack); > > In that case the recursive BUG() in track_stack() will happen anyway. You know, > I would better make CONFIG_GCC_PLUGIN_STACKLEAK depend on CONFIG_VMAP_STACK. That turned out to be a bad idea. Unfortunately, VMAP_STACK is not available on x86_32, but STACKLEAK works on that platform. So I'll put the check behind #ifdef. Maybe having it is better than having a silent stack overflow. Best regards, Alexander
diff --git a/fs/exec.c b/fs/exec.c index 8333c4dce59b..8351369cd1e4 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1960,7 +1960,8 @@ void __used track_stack(void) current->thread.lowest_stack = sp; } - if (unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16))) + if (!IS_ENABLED(CONFIG_VMAP_STACK) && + unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16))) BUG(); } EXPORT_SYMBOL(track_stack);