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;