Re: [lockdep] b09be676e0 BUG: unable to handle kernel NULL pointer dereference at 000001f2

[Josh Poimboeuf]

On Tue, Oct 03, 2017 at 11:28:15AM -0500, Josh Poimboeuf wrote:
> There are two bugs:
> 
> 1) Somebody -- presumably lockdep -- is corrupting the stack.  Need the
>    lockdep people to look at that.
> 
> 2) The 32-bit FP unwinder isn't handling the corrupt stack very well,
>    It's blindly dereferencing untrusted data:
> 
>       /* Is the next frame pointer an encoded pointer to pt_regs? */
>       regs = decode_frame_pointer(next_bp);
>       if (regs) {
>               frame = (unsigned long *)regs;
>               len = regs_size(regs);
>               state->got_irq = true;
> 
>   On 32-bit, regs_size() dereferences the regs pointer before we know it
>   points to a valid stack.  I'll fix that, along with the other unwinder
>   improvements I discussed with Linus.

Tetsuo and/or Fengguang,

Would you mind testing with this patch?  It should at least prevent the
unwinder panic and should hopefully print a useful unwinder dump
instead.


diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index d145a0b1f529..a0dd4df2234d 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -44,7 +44,8 @@ static void unwind_dump(struct unwind_state *state)
                        state->stack_info.type, state->stack_info.next_sp,
                        state->stack_mask, state->graph_idx);
 
-       for (sp = state->orig_sp; sp; sp = PTR_ALIGN(stack_info.next_sp, 
sizeof(long))) {
+       for (sp = PTR_ALIGN(state->orig_sp, sizeof(long)); sp;
+            sp = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
                if (get_stack_info(sp, state->task, &stack_info, &visit_mask))
                        break;
 
@@ -77,6 +78,12 @@ static size_t regs_size(struct pt_regs *regs)
        return sizeof(*regs);
 }
 
+#ifdef CONFIG_X86_32
+#define KERNEL_REGS_SIZE (sizeof(struct pt_regs) - 2*sizeof(long))
+#else
+#define KERNEL_REGS_SIZE (sizeof(struct pt_regs))
+#endif
+
 static bool in_entry_code(unsigned long ip)
 {
        char *addr = (char *)ip;
@@ -178,7 +185,7 @@ static struct pt_regs *decode_frame_pointer(unsigned long 
*bp)
 {
        unsigned long regs = (unsigned long)bp;
 
-       if (!(regs & 0x1))
+       if ((regs & (sizeof(long)-1)) != 1)
                return NULL;
 
        return (struct pt_regs *)(regs & ~0x1);
@@ -202,7 +209,7 @@ static bool update_stack_state(struct unwind_state *state,
        regs = decode_frame_pointer(next_bp);
        if (regs) {
                frame = (unsigned long *)regs;
-               len = regs_size(regs);
+               len = KERNEL_REGS_SIZE;
                state->got_irq = true;
        } else {
                frame = next_bp;
@@ -221,11 +228,23 @@ static bool update_stack_state(struct unwind_state *state,
                                   &state->stack_mask))
                        return false;
 
+       /* Make sure the frame pointer is properly aligned: */
+       if ((unsigned long)frame & (sizeof(long)-1))
+               return false;
+
        /* Make sure it only unwinds up and doesn't overlap the prev frame: */
        if (state->orig_sp && state->stack_info.type == prev_type &&
            frame < prev_frame_end)
                return false;
 
+       /*
+        * On 32-bit with user mode regs, make sure the last two regs are safe
+        * to access:
+        */
+       if (IS_ENABLED(CONFIG_X86_32) && regs && user_mode(regs) &&
+           !on_stack(info, frame, len + 2*sizeof(long)))
+               return false;
+
        /* Move state to the next frame: */
        if (regs) {
                state->regs = regs;