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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <[email protected]>

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 <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

#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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux