From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755423AbcHSS1X (ORCPT ); Fri, 19 Aug 2016 14:27:23 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:36418 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752088AbcHSS1U (ORCPT ); Fri, 19 Aug 2016 14:27:20 -0400 MIME-Version: 1.0 In-Reply-To: <62fab36288792edae0181274641d6b4c62157fea.1471525031.git.jpoimboe@redhat.com> References: <62fab36288792edae0181274641d6b4c62157fea.1471525031.git.jpoimboe@redhat.com> From: Kees Cook Date: Fri, 19 Aug 2016 11:27:18 -0700 X-Google-Sender-Auth: RlJc0gFLwFeplEPP5yE3ghstvx8 Message-ID: Subject: Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder To: Josh Poimboeuf Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , "x86@kernel.org" , LKML , Andy Lutomirski , Linus Torvalds , Steven Rostedt , Brian Gerst , Peter Zijlstra , Frederic Weisbecker , Byungchul Park , Nilay Vaish Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf wrote: > Convert arch_within_stack_frames() to use the new unwinder. > > This also changes some existing behavior: > > - Skip checking of pt_regs frames. > - Warn if it can't reach the grandparent's stack frame. > - Warn if it doesn't unwind to the end of the stack. > > Signed-off-by: Josh Poimboeuf All the stuff touching usercopy looks good to me. One question, though, in looking through the unwinder. It seems like it's much more complex than just the frame-hopping that the old arch_within_stack_frames() did, but I'm curious to hear what you think about its performance. We'll be calling this with every usercopy that touches the stack, so I'd like to be able to estimate the performance impact of this replacement... -Kees > --- > arch/x86/lib/usercopy.c | 44 ++++++++++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c > index 2492fa7..8fe0a9c 100644 > --- a/arch/x86/lib/usercopy.c > +++ b/arch/x86/lib/usercopy.c > @@ -50,30 +50,42 @@ int arch_within_stack_frames(const void * const stack, > const void * const stackend, > const void *obj, unsigned long len) > { > - const void *frame = NULL; > - const void *oldframe; > + struct unwind_state state; > + const void *frame, *frame_end; > + > + /* > + * Start at the end of our grandparent's frame (beginning of > + * great-grandparent's frame). > + */ > + unwind_start(&state, current, NULL, NULL); > + if (WARN_ON_ONCE(!unwind_next_frame(&state) || > + !unwind_next_frame(&state))) > + return 0; > + frame = unwind_get_stack_ptr(&state); > > - oldframe = __builtin_frame_address(2); > - if (oldframe) > - frame = __builtin_frame_address(3); > /* > * low ----------------------------------------------> high > * [saved bp][saved ip][args][local vars][saved bp][saved ip] > * ^----------------^ > * allow copies only within here > */ > - while (stack <= frame && frame < stackend) { > - /* > - * If obj + len extends past the last frame, this > - * check won't pass and the next frame will be 0, > - * causing us to bail out and correctly report > - * the copy as invalid. > - */ > - if (obj + len <= frame) > - return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1; > - oldframe = frame; > - frame = *(const void * const *)frame; > + frame += 2*sizeof(long); > + > + while (unwind_next_frame(&state)) { > + frame_end = unwind_get_stack_ptr(&state); > + > + /* skip checking of pt_regs frames */ > + if (!unwind_get_entry_regs(&state) && > + obj >= frame && obj + len <= frame_end) > + return 1; > + > + frame = frame_end + 2*sizeof(long); > } > + > + /* make sure the unwinder reached the end of the task stack */ > + if (WARN_ON_ONCE(frame != (void *)task_pt_regs(current))) > + return 0; > + > return -1; > } > #endif /* CONFIG_HARDENED_USERCOPY && CONFIG_FRAME_POINTER */ > -- > 2.7.4 > -- Kees Cook Nexus Security