lkml.org 
[lkml]   [2016]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access
    On Mon, Aug 15, 2016 at 10:22 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
    > 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 '&regs->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
    >> > > '&regs->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. &regs->sp really is the previous stack pointer in the
    >> > sense that the stack pointer was &regs->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.

    Yes, especially since your code seems to know how to find the previous
    stack already.

    >
    > However I think fixing that is outside the scope of this
    > already-way-too-big patch set.

    Agreed.

    >
    > --
    > Josh



    --
    Andy Lutomirski
    AMA Capital Management, LLC

    \
     
     \ /
      Last update: 2016-09-17 09:57    [W:4.040 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site