Messages in this thread | | | Date | Mon, 15 Aug 2016 12:22:52 -0500 | From | Josh Poimboeuf <> | Subject | Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access |
| |
On Mon, Aug 15, 2016 at 10:05:58AM -0500, Josh Poimboeuf wrote: > On Sun, Aug 14, 2016 at 12:26:29AM -0700, Andy Lutomirski wrote: > > On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On x86_32, when an interrupt happens from kernel space, SS and SP aren't > > > pushed and the existing stack is used. So pt_regs is effectively two > > > words shorter, and the previous stack pointer is normally the memory > > > after the shortened pt_regs, aka '®s->sp'. > > > > > > But in the rare case where the interrupt hits right after the stack > > > pointer has been changed to point to an empty stack, like for example > > > when call_on_stack() is used, the address immediately after the > > > shortened pt_regs is no longer on the stack. In that case, instead of > > > '®s->sp', the previous stack pointer should be retrieved from the > > > beginning of the current stack page. > > > > > > kernel_stack_pointer() wants to do that, but it forgets to dereference > > > the pointer. So instead of returning a pointer to the previous stack, > > > it returns a pointer to the beginning of the current stack. > > > > > > Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure") > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > > > > This seems like a valid fix, but I'm not sure I agree with the intent > > of the code. ®s->sp really is the previous stack pointer in the > > sense that the stack pointer was ®s->sp when the entry happened. > > From an unwinder's perspective, how is: > > > > movl [whatever], $esp > > <-- interrupt > > > > any different from: > > > > movl [whatever], $esp > > pushl [something] > > <-- interrupt > > In the first case, the stack is empty, so reading the value pointed to > by %esp would result in accessing outside the bounds of the stack.
...but maybe your point is that following the previous stack pointer is outside the scope of kernel_stack_pointer() and should instead be done by its caller. Especially considering the fact that the x86_64 version of this function doesn't have this "feature". In which case I think I would agree.
However I think fixing that is outside the scope of this already-way-too-big patch set.
-- Josh
| |