• Alexander Nyberg's avatar
    [PATCH] x86 stack initialisation fix · f48d9663
    Alexander Nyberg authored
    The recent change fix-crash-in-entrys-restore_all.patch
    
     	childregs->esp = esp;
    
     	p->thread.esp = (unsigned long) childregs;
    -	p->thread.esp0 = (unsigned long) (childregs+1);
    +	p->thread.esp0 = (unsigned long) (childregs+1) - 8;
    
     	p->thread.eip = (unsigned long) ret_from_fork;
    
    introduces an inconsistency between esp and esp0 before the task is run the
    first time.  esp0 is no longer the actual start of the stack, but 8 bytes
    off.
    
    This shows itself clearly in a scenario when a ptracer that is set to also
    ptrace eventual children traces program1 which then clones thread1.  Now
    the ptracer wants to modify the registers of thread1.  The x86 ptrace
    implementation bases it's knowledge about saved user-space registers upon
    p->thread.esp0.  But this will be a few bytes off causing certain writes to
    the kernel stack to overwrite a saved kernel function address making the
    kernel when actually running thread1 jump out into user-space.  Very
    spectacular.
    
    The testcase I've used is:
    /* start with strace -f ./a.out */
    #include <pthread.h>
    #include <stdio.h>
    
    void *do_thread(void *p)
    {
    	for (;;);
    }
    
    int main()
    {
    	pthread_t one;
    	pthread_create(&one, NULL, &do_thread, NULL);
    	for (;;);
    	return 0;
    }
    
    So, my solution is to instead of just adjusting esp0 that creates an
    inconsitent state I adjust where the user-space registers are saved with -8
    bytes.  This gives us the wanted extra bytes on the start of the stack and
    esp0 is now correct.  This solves the issues I saw from the original
    testcase from Mateusz Berezecki and has survived testing here.  I think
    this should go into -mm a round or two first however as there might be some
    cruft around depending on pt_regs lying on the start of the stack.  That
    however would have broken with the first change too!
    
    It's actually a 2-line diff but I had to move the comment of why the -8 bytes
    are there a few lines up. Thanks to Zwane for helping me with this.
    Signed-off-by: default avatarAlexander Nyberg <alexn@telia.com>
    Signed-off-by: default avatarAndrew Morton <akpm@osdl.org>
    Signed-off-by: default avatarLinus Torvalds <torvalds@osdl.org>
    f48d9663
process.c 20.7 KB