i386: save registers before intra-privilege syscall

From: Philipp Kohlbecher <[email protected]>

The kernel_execve function issues a software interrupt (int 0x80) to make a system call to sys_execve. This function expects to find the stack segment and stack pointer of the function that issued the system call in the pt_regs struct. The syscall entry code that sets up this struct expects the stack segment and the stack pointer of the issuing function already on the stack. But the Intel processor saves these registers only if a stack-switch occurs, i.e. for inter-privilege interrupts and exceptions (cf. Intel Software Developer’s Manual, Vol. 3A, p. 5-17,
http://www.intel.com/design/processor/manuals/253668.pdf).
For an intra-privilege interrupt like the one issued in kernel_execve, these registers must be saved manually.

---

I am a total newbie and this is my first patch, so please be merciful... Also, please CC me, I am not on the list.

@@ -258,7 +258,8 @@ int kernel_execve(const char *filename,
 int kernel_execve(const char *filename, char *const argv[], char *const envp[])
 {
 	long __res;
-	asm volatile ("push %%ebx ; movl %2,%%ebx ; int $0x80 ; pop %%ebx"
+	asm volatile ("push %%ebx ; movl %2,%%ebx ; push %%ss ; push %%esp;"
+				"int $0x80 ; addl $(4*2),%%esp ; pop %%ebx"
 	: "=a" (__res)
 	: "0" (__NR_execve),"ri" (filename),"c" (argv), "d" (envp) : "memory");
 	return __res;

-
To unsubscribe from this list: send the line "unsubscribe linux-assembly" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Could you describe the failure scenario this causes? I'm trying to understand how something that fundamental would have possibly slipped by testing?

-hpa

I don't know of any problems this causes. The kernel needs to be aware of the fact that the xss and esp fields of the pt_regs struct may contain wrong values anyway, as hardware interrupts arriving while the CPU is in kernel mode would also lead to this condition.
The file include/asm-i386/processor.h contains a comment to that effect (lines 483-492).
With kernel_execve we can predict this, however, and account for it. (This may be superfluous, but I don't think it hurts and it might prevent future errors.)

- Phil Kohlbecher
-
To unsubscribe from this list: send the line "unsubscribe linux-assembly" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

... and it may *cause* future errors by making it harder to find bugs, too.

In other words, your patch doesn't actually fix anything, it *masks* potential bugs which would also be triggered by interrupts in kernel mode. This is bad.

You realize you just make a distinction between synchronous and asynchronous events, such that your patch means that only asynchronous events would catch the bugs you mask. This makes bug hunting much harder.

If anything it would be better to poison those fields so that we can smoke out anything that trusts them.

-hpa

I am not sure these potential bugs would also be triggered by interrupts in kernel mode. After all, kernel_execve is a special case: It is the only case (AFAIK) where a software interrupt is generated while in kernel mode (intra-privilege interrupt), but (if all goes right) the iret "returns" to user mode (inter-privilege return).

This iret will pop the ss and esp registers off the stack, so start_thread (include/asm-i386/processor.h:444) needs to put the correct values in these stack positions, which it does.

Now, my point is that, when start_thread gets called at the end of a successful kernel_execve, these stack positions cannot contain what they are supposed to contain and no space is allocated on the stack for these register values, so that something else gets overwritten.

It would seem cleaner to handle this special case differently by making sure that the stack is set up the way it would be for an inter-privilege interrupt.

- Phil Kohlbecher

Yes, and it *BETTER*, because those are user-mode values which refer to the newly created task.

Sticking kernel mode values in those fields would add no value, except as a poison (since %ss == KERNEL_DS and would cause a #GP(0) if it ever reached IRET.) If anything, those fields should be pushed as zero or some other poison bits. That would be slightly better than what's there now, which is whatever garbage happens to be on the stack. Pushing the kernel SS:ESP is just plain wrong (not to mention that the way you do it doesn't even produce the right value for ESP -- you'd have to save away ESP before you push SS.)

-hpa

#SS(0), rather, of course.

-hpa

That's true. The xss and esp fields of the pt_regs struct always contain either garbage (for interrupts occuring while in kernel mode) or user-mode values (for interrupts occuring while in user mode). So, filling these fields with kernel-mode values indeed doesn't make much sense.

Allocating space on the stack and poisoning those values would make sense, though, so I will modify the patch accordingly and resend it.

Thank you for your feedback!

- Philipp Kohlbecher