Re: [PATCH] Fix out-by-one error in traps.c

[Linus Torvalds]

On Sat, 1 Sep 2007, Rusty Russell wrote:
> 
> This is only for the initial booting stack (init_thread_union); see
> arch/i386/kernel/head.S:
> 	/* Set up the stack pointer */
> 	lss stack_start,%esp
> 	...
> 	pushl $0		# fake return address for unwinder

Ok, we should fix that. We should just make it look like all other stack 
frames.

There is other code in the kernel that "knows" that all kernel stacks have 
the fields for the user stack return on it, namely the ptrace code etc. 
Now, the initial stack is hopefully never *accessed* by that kind of code, 
but this kind of special-case code is just wrong.

> > But your patch does improve the sanity checking of the frame pointer. That 
> > said, I think the following patch improves it more: does this also work 
> > for you? (Totally untested, but it looks like the RightThing(tm) to do)
> 
> Yes, looks good.  Perhaps one additional magic num removal:

Well, we might as well then just make the code readable instead. IOW, how 
about this one, which just declares a structure that describes the stack 
frame thing? That just makes everything clearer, since we can then use 
"sizeof(that structure)" instead of using the magic "2*sizeof(unsigned 
long)".

		Linus

---
diff --git a/arch/i386/kernel/traps.c b/arch/i386/kernel/traps.c
index cfffe3d..47b0bef 100644
--- a/arch/i386/kernel/traps.c
+++ b/arch/i386/kernel/traps.c
@@ -100,36 +100,45 @@ asmlinkage void machine_check(void);
 int kstack_depth_to_print = 24;
 static unsigned int code_bytes = 64;
 
-static inline int valid_stack_ptr(struct thread_info *tinfo, void *p)
+static inline int valid_stack_ptr(struct thread_info *tinfo, void *p, unsigned size)
 {
 	return	p > (void *)tinfo &&
-		p < (void *)tinfo + THREAD_SIZE - 3;
+		p <= (void *)tinfo + THREAD_SIZE - size;
 }
 
+/* The form of the top of the frame on the stack */
+struct stack_frame {
+	struct stack_frame *next_frame;
+	unsigned long return_address;
+};
+
 static inline unsigned long print_context_stack(struct thread_info *tinfo,
 				unsigned long *stack, unsigned long ebp,
 				struct stacktrace_ops *ops, void *data)
 {
-	unsigned long addr;
-
 #ifdef	CONFIG_FRAME_POINTER
-	while (valid_stack_ptr(tinfo, (void *)ebp)) {
-		unsigned long new_ebp;
-		addr = *(unsigned long *)(ebp + 4);
+	struct stack_frame *frame = (struct stack_frame *)ebp;
+	while (valid_stack_ptr(tinfo, frame, sizeof(*frame))) {
+		struct stack_frame *next;
+		unsigned long addr;
+
+		addr = frame->return_address;
 		ops->address(data, addr);
 		/*
 		 * break out of recursive entries (such as
 		 * end_of_stack_stop_unwind_function). Also,
 		 * we can never allow a frame pointer to
 		 * move downwards!
-	 	 */
-	 	new_ebp = *(unsigned long *)ebp;
-		if (new_ebp <= ebp)
+		 */
+		next = frame->next_frame;
+		if (next <= frame)
 			break;
-		ebp = new_ebp;
+		frame = next;
 	}
 #else
-	while (valid_stack_ptr(tinfo, stack)) {
+	while (valid_stack_ptr(tinfo, stack, sizeof(*stack))) {
+		unsigned long addr;
+
 		addr = *stack++;
 		if (__kernel_text_address(addr))
 			ops->address(data, addr);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/