Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs - Kernel

This is a discussion on Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs - Kernel ; On Wed, 03 Oct 2007 17:06:24 +0900, Shi Weihua wrote: > Fixing alternative signal stack wraparound. > > If a process uses alternative signal stack by using sigaltstack() > and that stack overflow, stack wraparound occurs. > This patch checks ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

  1. Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

    On Wed, 03 Oct 2007 17:06:24 +0900, Shi Weihua wrote:
    > Fixing alternative signal stack wraparound.
    >
    > If a process uses alternative signal stack by using sigaltstack()
    > and that stack overflow, stack wraparound occurs.
    > This patch checks whether the signal frame is on the alternative
    > stack. If the frame is not on there, kill a signal SIGSEGV to the process forcedly
    > then the process will be terminated.
    >
    > This patch is for i386,version is 2.6.23-rc8.
    >
    > Signed-off-by: Shi Weihua
    >
    > diff -pur linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c linux-2.6.23-rc8/arch/i386/kernel/signal.c
    > --- linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c 2007-09-26 09:44:08.000000000 +0900
    > +++ linux-2.6.23-rc8/arch/i386/kernel/signal.c 2007-09-26 13:14:25.000000000 +0900
    > @@ -332,6 +332,10 @@ static int setup_frame(int sig, struct k
    >
    > frame = get_sigframe(ka, regs, sizeof(*frame));
    >
    > + if ((ka->sa.sa_flags & SA_ONSTACK) &&
    > + !sas_ss_flags((unsigned long)frame))
    > + goto give_sigsegv;
    > +
    > if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
    > goto give_sigsegv;
    >
    > @@ -425,6 +429,10 @@ static int setup_rt_frame(int sig, struc
    >
    > frame = get_sigframe(ka, regs, sizeof(*frame));
    >
    > + if ((ka->sa.sa_flags & SA_ONSTACK) &&
    > + !sas_ss_flags((unsigned long)frame))
    > + goto give_sigsegv;
    > +
    > if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
    > goto give_sigsegv;


    Your patch description is a little terse. What you do is that
    after the kernel has decided where to put the signal frame,
    you add a check that the base of the frame still lies in the
    altstack range if altstack delivery is requested for the signal,
    and if it doesn't a hard error is forced.

    The coding of that logic is fine.

    What I don't agree with is the logic itself:
    - You only catch altstack overflow caused by the kernel pushing
    a sigframe. You don't catch overflow caused by the user-space
    signal handler pushing its own stack frame after the sigframe.
    - SUSv3 specifies the effect of altstack overflow as "undefined".
    - The overflow problem can be solved in user-space: allocate the
    altstack with mmap(), then mprotect() the lowest page to prevent
    accesses to it. Any overflow into it, by the kernel's signal
    delivery code or by the user-space signal handler, will be caught.

    So this patch gets a NAK from me.

    /Mikael
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

    On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
    Mikael Pettersson wrote:

    > What I don't agree with is the logic itself:
    > - You only catch altstack overflow caused by the kernel pushing
    > a sigframe. You don't catch overflow caused by the user-space
    > signal handler pushing its own stack frame after the sigframe.
    > - SUSv3 specifies the effect of altstack overflow as "undefined".
    > - The overflow problem can be solved in user-space: allocate the
    > altstack with mmap(), then mprotect() the lowest page to prevent
    > accesses to it. Any overflow into it, by the kernel's signal
    > delivery code or by the user-space signal handler, will be caught.
    >
    > So this patch gets a NAK from me.
    >


    I can understand what you say, but a program which meets this problem
    cannot be debugged ;(

    gdb just shows infinit loop of function frames and origignal signal frame
    which includes the most important information is overwritten.

    Ah yes, user's sigaltstack setup is bad if this happens, but I can't ask
    novice programmers to take care of "please verify the page next to
    sigaltstack is not mapped or protected." such a thing is not written in man(2)
    page of sigaltstack now.


    Thanks,
    -Kame
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

    On Wed, 3 Oct 2007 21:40:29 +0900
    KAMEZAWA Hiroyuki wrote:

    > On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
    > Mikael Pettersson wrote:
    >
    > > What I don't agree with is the logic itself:
    > > - You only catch altstack overflow caused by the kernel pushing
    > > a sigframe. You don't catch overflow caused by the user-space
    > > signal handler pushing its own stack frame after the sigframe.
    > > - SUSv3 specifies the effect of altstack overflow as "undefined".
    > > - The overflow problem can be solved in user-space: allocate the
    > > altstack with mmap(), then mprotect() the lowest page to prevent
    > > accesses to it. Any overflow into it, by the kernel's signal
    > > delivery code or by the user-space signal handler, will be caught.
    > >
    > > So this patch gets a NAK from me.
    > >

    >
    > I can understand what you say, but a program which meets this problem
    > cannot be debugged ;(
    >
    > gdb just shows infinit loop of function frames and origignal signal frame
    > which includes the most important information is overwritten.
    >

    there is a difference among user's stack overflow and kernel's.
    - user's stack overflow just breaks memory next to stack frame.
    - kernel's altstack overflow, which this patch tries to fix, breaks
    the bottom of altstack bacause %esp goes back to the bottom
    of ths altstack when it exceeds altstack range.
    This behavior overwrite orignail stack frame and shows infinit loop
    of function call to gdb and never stop with 100% cpu usage.

    Thanks,
    -Kame
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

    Mikael Pettersson wrote::
    > On Wed, 03 Oct 2007 17:06:24 +0900, Shi Weihua wrote:
    >> Fixing alternative signal stack wraparound.
    >>
    >> If a process uses alternative signal stack by using sigaltstack()
    >> and that stack overflow, stack wraparound occurs.
    >> This patch checks whether the signal frame is on the alternative
    >> stack. If the frame is not on there, kill a signal SIGSEGV to the process forcedly
    >> then the process will be terminated.
    >>
    >> This patch is for i386,version is 2.6.23-rc8.
    >>
    >> Signed-off-by: Shi Weihua
    >>
    >> diff -pur linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c linux-2.6.23-rc8/arch/i386/kernel/signal.c
    >> --- linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c 2007-09-26 09:44:08.000000000 +0900
    >> +++ linux-2.6.23-rc8/arch/i386/kernel/signal.c 2007-09-26 13:14:25.000000000 +0900
    >> @@ -332,6 +332,10 @@ static int setup_frame(int sig, struct k
    >>
    >> frame = get_sigframe(ka, regs, sizeof(*frame));
    >>
    >> + if ((ka->sa.sa_flags & SA_ONSTACK) &&
    >> + !sas_ss_flags((unsigned long)frame))
    >> + goto give_sigsegv;
    >> +
    >> if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
    >> goto give_sigsegv;
    >>
    >> @@ -425,6 +429,10 @@ static int setup_rt_frame(int sig, struc
    >>
    >> frame = get_sigframe(ka, regs, sizeof(*frame));
    >>
    >> + if ((ka->sa.sa_flags & SA_ONSTACK) &&
    >> + !sas_ss_flags((unsigned long)frame))
    >> + goto give_sigsegv;
    >> +
    >> if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
    >> goto give_sigsegv;

    >
    > Your patch description is a little terse. What you do is that
    > after the kernel has decided where to put the signal frame,
    > you add a check that the base of the frame still lies in the
    > altstack range if altstack delivery is requested for the signal,
    > and if it doesn't a hard error is forced.
    >
    > The coding of that logic is fine.
    >
    > What I don't agree with is the logic itself:
    > - You only catch altstack overflow caused by the kernel pushing
    > a sigframe. You don't catch overflow caused by the user-space
    > signal handler pushing its own stack frame after the sigframe.
    > - SUSv3 specifies the effect of altstack overflow as "undefined".
    > - The overflow problem can be solved in user-space: allocate the
    > altstack with mmap(), then mprotect() the lowest page to prevent
    > accesses to it. Any overflow into it, by the kernel's signal
    > delivery code or by the user-space signal handler, will be caught.


    mmap/mprotect can not avoid this kind of wraparound.
    Please compile and run the following test code on i386.
    The code want to allow process access from high to mid,and not from mid to low.
    high
    |
    |
    mid
    |
    |
    low

    #include
    #include
    #include
    #include
    #include
    #include

    #define die(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)
    volatile int counter = 0;

    #ifdef __i386__
    void print_esp()
    {
    unsigned long esp;
    __asm__ __volatile__("movl %%esp, %0":"=g"(esp));

    printf("esp = 0x%08lx\n", esp);
    }
    #endif

    static void segv_handler()
    {
    #ifdef __i386__
    print_esp();
    #endif

    int *c = NULL;
    counter++;
    printf("%d\n", counter);

    *c = 1; // SEGV
    }

    int main()
    {
    int *c = NULL;
    int pagesize;
    char *addr;
    stack_t stack;
    struct sigaction action;

    pagesize = sysconf(_SC_PAGE_SIZE);
    if (pagesize == -1) {
    die("sysconf");
    exit(EXIT_FAILURE);
    }

    addr = mmap(NULL, pagesize * 2, PROT_READ | PROT_WRITE | PROT_EXEC,
    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    if (addr == MAP_FAILED) {
    die("mmap");
    exit(EXIT_FAILURE);
    }
    printf("begin = 0x%08lx\n", addr);
    printf("end = 0x%08lx\n", addr + pagesize * 2);

    if (mprotect(addr, pagesize, PROT_NONE) == -1) {
    die("mprotect");
    exit(EXIT_FAILURE);
    }

    stack.ss_sp = addr + pagesize;
    stack.ss_flags = 0;
    stack.ss_size = pagesize; //SIGSTKSZ;
    int error = sigaltstack(&stack, NULL);
    if (error) {
    printf("Failed to use sigaltstack!\n");
    return -1;
    }

    memset(&action, 0, sizeof(action));
    action.sa_handler = segv_handler;
    action.sa_flags = SA_ONSTACK | SA_NODEFER;

    sigemptyset(&action.sa_mask);

    sigaction(SIGSEGV, &action, NULL);

    *c = 0; //SEGV

    return 0;
    }

    Any suggestion?

    Thanks
    Shi Weihua

    >
    > So this patch gets a NAK from me.
    >
    > /Mikael
    >
    >
    >


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread