| Message ID | 5899FDDD.3080605@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 7, 2017 at 9:03 AM, James Morse <james.morse@arm.com> wrote: > Hi Sahara, > > On 07/02/17 10:19, James Morse wrote: >> On 05/02/17 12:14, kpark3469@gmail.com wrote: >>> From: Sahara <keun-o.park@darkmatter.ae> >>> >>> This implements arch_within_stack_frames() for arm64 that should >>> validate if a given object is contained by a kernel stack frame. >>> >>> Signed-off-by: Sahara <keun-o.park@darkmatter.ae> > >> I'd like to avoid having two sets of code that walk the stack. >> I will have a go at a version of this patch that uses arm64s existing >> walk_stackframe() machinery - lets find out if there is a good reason not to do >> it that way! > > The reason turns out to be because LKDTM isn't testing whether we are > overlapping stack frames. > Instead it wants us to tell it whether the original caller somewhere down the > stack pointed into a stack frame that hadn't yet been written. This requires > this function to know how it will be called and unwind some number of frames. > Annoyingly we have to maintain start/end boundaries for each frame in case the > object was neatly contained in a frame that wasn't written at the time. "hadn't yet been written"? This doesn't make sense. The hardened usercopy stack frame check (which is what LKDTM is exercising) wants to simply walk from the current frame up, making sure that the object in question is entirely contained by any single stack frame. Any dynamic stack allocations should already be covered since it would be within the caller's frame. Am I missing something? > Ideally we would inline this stack check into the caller, testing object_end > doesn't lie in the range current_stack_pointer : end_of_stack. This would work > even when built without CONFIG_FRAME_POINTER but is probably too-invasive a change. > > As is, the walk_stackframe() machinery version looks like this: > ---------------------------------------%<--------------------------------------- > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 8a552a33c6ef..620fa74201b5 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -25,6 +25,12 @@ > #include <asm/stack_pointer.h> > #include <asm/stacktrace.h> > > +#define FAKE_FRAME(frame, my_func) do { \ > + frame.fp = (unsigned long)__builtin_frame_address(0); \ > + frame.sp = current_stack_pointer; \ > + frame.pc = (unsigned long)my_func; \ > +} while (0) > + > /* > * AArch64 PCS assigns the frame pointer to x29. > * > @@ -194,9 +200,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct > stack_trace *trace) > frame.pc = thread_saved_pc(tsk); > } else { > data.no_sched_functions = 0; > - frame.fp = (unsigned long)__builtin_frame_address(0); > - frame.sp = current_stack_pointer; > - frame.pc = (unsigned long)save_stack_trace_tsk; > + FAKE_FRAME(frame, save_stack_trace_tsk); > } > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > frame.graph = tsk->curr_ret_stack; > @@ -215,3 +219,73 @@ void save_stack_trace(struct stack_trace *trace) > } > EXPORT_SYMBOL_GPL(save_stack_trace); > #endif > + > +struct check_frame_arg { > + unsigned long obj_start; > + unsigned long obj_end; > + unsigned long frame_start; > + int discard_frames; > + int err; > +}; > + > +static int check_frame(struct stackframe *frame, void *d) > +{ > + struct check_frame_arg *arg = d; > + unsigned long frame_end = frame->fp; > + > + /* object overlaps multiple frames */ > + if (arg->obj_start < frame->fp && frame->fp < arg->obj_end) { > + arg->err = BAD_STACK; > + return 1; > + } > + > + /* > + * Discard frames and check object is in a frame written early > + * enough. > + */ > + if (arg->discard_frames) > + arg->discard_frames--; > + else if ((arg->frame_start <= arg->obj_start && arg->obj_start < > frame_end) && > + (arg->frame_start < arg->obj_end && arg->obj_end <= frame_end)) > + return 1; > + > + /* object exists in a previous frame */ > + if (arg->obj_end < arg->frame_start) { > + arg->err = BAD_STACK; > + return 1; > + } > + > + arg->frame_start = frame_end + 0x10; > + > + return 0; > +} > + > +/* Check obj doesn't overlap a stack frame record */ > +enum stack_type arch_within_stack_frames(const void *stack, > + const void *stack_end, > + const void *obj, unsigned long obj_len) > +{ > + struct stackframe frame; > + struct check_frame_arg arg; > + > + if (!IS_ENABLED(CONFIG_FRAME_POINTER)) > + return NOT_STACK; > + > + arg.err = GOOD_FRAME; > + arg.obj_start = (unsigned long)obj; > + arg.obj_end = arg.obj_start + obj_len; > + > + FAKE_FRAME(frame, arch_within_stack_frames); > + arg.frame_start = frame.fp; > + > + /* > + * Skip 4 non-inlined frames: <fake frame>, > + * arch_within_stack_frames(), check_stack_object() and > + * __check_object_size(). > + */ > + arg.discard_frames = 4; > + > + walk_stackframe(current, &frame, check_frame, &arg); > + > + return arg.err; > +} > ---------------------------------------%<--------------------------------------- > > This avoids the __builtin_frame_address(!0) problem and doesn't duplicate the > actual stack walking. This can be simplified further if we can get rid of the > time-travel requirements. > > It is unfortunately more code, but it is hopefully clearer! This doesn't seem to sanity-check that the frame is still within the process stack. We'd want to make sure it can't walk off into la-la land. :) (We could just add "stack" and "stack_end" to the check_frame_arg struct along with checks?) -Kees
Hi Kees, On 07/02/17 18:13, Kees Cook wrote: > On Tue, Feb 7, 2017 at 9:03 AM, James Morse <james.morse@arm.com> wrote: >> On 07/02/17 10:19, James Morse wrote: >> The reason turns out to be because LKDTM isn't testing whether we are >> overlapping stack frames. >> Instead it wants us to tell it whether the original caller somewhere down the >> stack pointed into a stack frame that hadn't yet been written. This requires >> this function to know how it will be called and unwind some number of frames. >> Annoyingly we have to maintain start/end boundaries for each frame in case the >> object was neatly contained in a frame that wasn't written at the time. > > "hadn't yet been written"? This doesn't make sense. Sorry, "wasn't contained by a frame at the time copy_to_user() was called, even if it is now...". > The hardened > usercopy stack frame check (which is what LKDTM is exercising) wants > to simply walk from the current frame up, making sure that the object > in question is entirely contained by any single stack frame. Any > dynamic stack allocations should already be covered since it would be > within the caller's frame. Sure, maybe I'm looking at the wrong lkdtm test then. I see this happening: do_usercopy_stack_callee() returns its own stack value (while trying to confuse the compiler). We know this value must be after do_usercopy_stack()s frame. do_usercopy_stack() then passes this value to copy_{to,from}_user(), the test expects this to to be rejected. copy_{to,from}_user() then inline a call to __check_object_size(), which in turn calls check_stack_object() (which is marked noinline). These calls will generate stack frames, which will overlap the value do_usercopy_stack_callee() returned. By the time arch_within_stack_frames() is called, the value returned by do_usercopy_stack_callee() is within a stack frame. It just wasn't within a stack frame at the time copy_to_user() was called. Does this make sense, or have I gone off the rails? One way to fix this is to make the size given to copy_to_user() so large that it must overlap multiple stack frames. 32 bytes is too small given arm64 kernel stacks have to be 16 byte aligned. A better trick would be to inline the 'not after our stack frame' check into do_usercopy_stack(), but that means exposing the report_usercopy() and maybe some more. (I will give it a go). > This doesn't seem to sanity-check that the frame is still within the > process stack. We'd want to make sure it can't walk off into la-la > land. :) (We could just add "stack" and "stack_end" to the > check_frame_arg struct along with checks?) The arch unwind_frame() machinery does this for us, in particular the cryptic: > if (fp < low || fp > high || fp & 0xf) > return -EINVAL; Is testing that the freshly read 'fp' is between the 'top' of this frame and the 'bottom' of the stack. The only corner case would be if you called this and object wasn't on the stack at all to begin with, but core code already checks this. Before calling arch_within_stack_frames(), mm/usercopy.c:check_stack_object(): > /* Object is not on the stack at all. */ > if (obj + len <= stack || stackend <= obj) > return NOT_STACK; Thanks, James
On Wed, Feb 8, 2017 at 3:16 AM, James Morse <james.morse@arm.com> wrote: > On 07/02/17 18:13, Kees Cook wrote: >> On Tue, Feb 7, 2017 at 9:03 AM, James Morse <james.morse@arm.com> wrote: >>> On 07/02/17 10:19, James Morse wrote: >>> The reason turns out to be because LKDTM isn't testing whether we are >>> overlapping stack frames. >>> Instead it wants us to tell it whether the original caller somewhere down the >>> stack pointed into a stack frame that hadn't yet been written. This requires >>> this function to know how it will be called and unwind some number of frames. >>> Annoyingly we have to maintain start/end boundaries for each frame in case the >>> object was neatly contained in a frame that wasn't written at the time. >> >> "hadn't yet been written"? This doesn't make sense. > > Sorry, "wasn't contained by a frame at the time copy_to_user() was called, even > if it is now...". > >> The hardened >> usercopy stack frame check (which is what LKDTM is exercising) wants >> to simply walk from the current frame up, making sure that the object >> in question is entirely contained by any single stack frame. Any >> dynamic stack allocations should already be covered since it would be >> within the caller's frame. > > Sure, maybe I'm looking at the wrong lkdtm test then. I see this happening: > > do_usercopy_stack_callee() returns its own stack value (while trying to confuse > the compiler). We know this value must be after do_usercopy_stack()s frame. > do_usercopy_stack() then passes this value to copy_{to,from}_user(), the test > expects this to to be rejected. > > copy_{to,from}_user() then inline a call to __check_object_size(), which in turn > calls check_stack_object() (which is marked noinline). These calls will generate > stack frames, which will overlap the value do_usercopy_stack_callee() returned. > > By the time arch_within_stack_frames() is called, the value returned by > do_usercopy_stack_callee() is within a stack frame. It just wasn't within a > stack frame at the time copy_to_user() was called. > > Does this make sense, or have I gone off the rails? That's true, but those frames should be ignored by the walker, and as such, should be rejected. (See below.) > One way to fix this is to make the size given to copy_to_user() so large that it > must overlap multiple stack frames. 32 bytes is too small given arm64 kernel > stacks have to be 16 byte aligned. > > A better trick would be to inline the 'not after our stack frame' check into > do_usercopy_stack(), but that means exposing the report_usercopy() and maybe > some more. (I will give it a go). Just to make sure I'm on the same page, the call stack is: do_usercopy_stack() (or anything calling the uaccess functions) copy_{to,from}_user() <- inlined into do_usercopy_stack() __check_object_size() check_stack_object() arch_within_stack_frames() <- inlined into check_stack_object() So, really, arch_within_stack_frames() should ignore the current frame (from check_stack_object()) and prior frame (from __check_object_size()), before starting its examination of frames. This is what the x86 walker does: oldframe = __builtin_frame_address(1); if (oldframe) frame = __builtin_frame_address(2); frame address 0 would be check_stack_object(), 1 would be __check_object_size(), so it starts its analysis at frame 2, which is do_usercopy_stack()'s frame, bounded by the end of __check_object_size()'s frame. Is there any reason the arm64 walker couldn't be identical to the x86 walker? >> This doesn't seem to sanity-check that the frame is still within the >> process stack. We'd want to make sure it can't walk off into la-la >> land. :) (We could just add "stack" and "stack_end" to the >> check_frame_arg struct along with checks?) > > The arch unwind_frame() machinery does this for us, in particular the cryptic: >> if (fp < low || fp > high || fp & 0xf) >> return -EINVAL; > > Is testing that the freshly read 'fp' is between the 'top' of this frame and the > 'bottom' of the stack. > > The only corner case would be if you called this and object wasn't on the stack > at all to begin with, but core code already checks this. Before calling > arch_within_stack_frames(), > mm/usercopy.c:check_stack_object(): >> /* Object is not on the stack at all. */ >> if (obj + len <= stack || stackend <= obj) >> return NOT_STACK; Cool, yeah. -Kees
Hi Kees, On 08/02/17 21:38, Kees Cook wrote: > On Wed, Feb 8, 2017 at 3:16 AM, James Morse <james.morse@arm.com> wrote: >> do_usercopy_stack_callee() returns its own stack value (while trying to confuse >> the compiler). We know this value must be after do_usercopy_stack()s frame. >> do_usercopy_stack() then passes this value to copy_{to,from}_user(), the test >> expects this to to be rejected. >> >> copy_{to,from}_user() then inline a call to __check_object_size(), which in turn >> calls check_stack_object() (which is marked noinline). These calls will generate >> stack frames, which will overlap the value do_usercopy_stack_callee() returned. >> >> By the time arch_within_stack_frames() is called, the value returned by >> do_usercopy_stack_callee() is within a stack frame. It just wasn't within a >> stack frame at the time copy_to_user() was called. >> >> Does this make sense, or have I gone off the rails? > > That's true, but those frames should be ignored by the walker, and as > such, should be rejected. (See below.) I think that's an odd thing for arch_within_stack_frames() to be doing. >> One way to fix this is to make the size given to copy_to_user() so large that it >> must overlap multiple stack frames. 32 bytes is too small given arm64 kernel >> stacks have to be 16 byte aligned. >> >> A better trick would be to inline the 'not after our stack frame' check into >> do_usercopy_stack(), but that means exposing the report_usercopy() and maybe >> some more. (I will give it a go). > > Just to make sure I'm on the same page, the call stack is: > > do_usercopy_stack() (or anything calling the uaccess functions) > copy_{to,from}_user() <- inlined into do_usercopy_stack() > __check_object_size() > check_stack_object() > arch_within_stack_frames() <- inlined into check_stack_object() I think this is where our world-view is different, I don't trust the compiler not to pull some surprising optimisation that inlines calls differently at different call-sites. The compiler won't always inline functions marked inline, [0] has some examples, (I'm not sure what it means by 'use of nested functions'!). Expecting a particular layout is fragile, Akashi's example shows gcc doesn't always place objects and the frame record where we expected. Requiring a particular layout for copy_to_user() to work is bordering on the 'sleepless nights' territory. > Is there any reason the arm64 walker couldn't be identical to the x86 walker? We would then have two stack walkers. In the light of Akashi's example, walking the stack and saying 'this object was allocated by this call' isn't something we can do, arch_within_stack_frames() shouldn't try. I have an alternate version of this patch that uses arm64s existing stack walker to look for fp appearing within an object, and another that tries to inline the bounds check into the caller. I will post these shortly for comparison... Thanks, James [0] https://gcc.gnu.org/onlinedocs/gcc/Inline.html
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 8a552a33c6ef..620fa74201b5 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -25,6 +25,12 @@ #include <asm/stack_pointer.h> #include <asm/stacktrace.h> +#define FAKE_FRAME(frame, my_func) do { \ + frame.fp = (unsigned long)__builtin_frame_address(0); \ + frame.sp = current_stack_pointer; \ + frame.pc = (unsigned long)my_func; \ +} while (0) + /* * AArch64 PCS assigns the frame pointer to x29. * @@ -194,9 +200,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) frame.pc = thread_saved_pc(tsk); } else { data.no_sched_functions = 0; - frame.fp = (unsigned long)__builtin_frame_address(0); - frame.sp = current_stack_pointer; - frame.pc = (unsigned long)save_stack_trace_tsk; + FAKE_FRAME(frame, save_stack_trace_tsk); } #ifdef CONFIG_FUNCTION_GRAPH_TRACER