execve() fails to report errors

PATCH Pavel Machek 4 participants 4 messages Mar 6, 2002 – Mar 15, 2002

Subsystems (2)

VFS exec and binfmt

lore

Share & tools

lore

Pavel Machek

author

2002-03-06 13:44 UTC #0 lore raw

Hi!

Take this trivial .c program. Obviously correct.

struct foo {
        char fill[1*1024*1024*1024];
};

struct foo a;

void
main(void)
{
}

Compile. Run. Segfault.

Whose fault? Kernels; it fails to corectly report not enough address space.

Now, I do not know if this is the right fix (binfmt_elf looks spaghetty to me) but its certainly better than it was.

Pavel

fs/binfmt_elf.c +16 -9

@@ -79,13 +79,19 @@
 
 #define BAD_ADDR(x)	((unsigned long)(x) > TASK_SIZE)
 
-static void set_brk(unsigned long start, unsigned long end)
+static int set_brk(unsigned long start, unsigned long end)
 {
+	int error;
 	start = ELF_PAGEALIGN(start);
 	end = ELF_PAGEALIGN(end);
 	if (end <= start)
-		return;
-	do_brk(start, end - start);
+		return 0;
+	
+	error = do_brk(start, end - start);
+	if (error!=start) {
+		return error;
+	}
+	return 0;
 }

@@ -606,7 +613,9 @@
 			/* There was a PT_LOAD segment with p_memsz > p_filesz
 			   before this one. Map anonymous pages, if needed,
 			   and clear the area.  */
-			set_brk (elf_bss + load_bias, elf_brk + load_bias);
+			retval = set_brk (elf_bss + load_bias, elf_brk + load_bias);
+			if (retval)
+				goto out_free_dentry;
 			nbyte = ELF_PAGEOFFSET(elf_bss);
 			if (nbyte) {
 				nbyte = ELF_MIN_ALIGN - nbyte;

@@ -721,8 +730,7 @@
 	/* Calling set_brk effectively mmaps the pages that we need
 	 * for the bss and break sections
 	 */
-	set_brk(elf_bss, elf_brk);
-
+	retval = set_brk(elf_bss, elf_brk);
 	padzero(elf_bss);
 
 #if 0

@@ -745,6 +753,7 @@
 		error = do_mmap(NULL, 0, 4096, PROT_READ | PROT_EXEC,
 				MAP_FIXED | MAP_PRIVATE, 0);
 		up_write(&current->mm->mmap_sem);
+		/* FIXME: Check return from mmap! */
 	}
 
 #ifdef ELF_PLAT_INIT

@@ -760,8 +769,7 @@
 	start_thread(regs, elf_entry, bprm->p);
 	if (current->ptrace & PT_PTRACED)
 		send_sig(SIGTRAP, current, 0);
-	retval = 0;
 out:
 	return retval;
 
 	/* error cleanup */

@@ -842,9 +852,9 @@
 
 	len = ELF_PAGESTART(elf_phdata->p_filesz + elf_phdata->p_vaddr + ELF_MIN_ALIGN - 1);
 	bss = elf_phdata->p_memsz + elf_phdata->p_vaddr;
-	if (bss > len)
-		do_brk(len, bss - len);
 	error = 0;
+	if (bss > len)
+		error = do_brk(len, bss - len);
 
 out_free_ph:
 	kfree(elf_phdata);

Signature · 2 lines

(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Richard B. Johnson

2002-03-06 14:09 UTC #1 lore raw

Quoted 16 lines · Pavel Machek

Hi!
Take this trivial .c program. Obviously correct.
struct foo {
        char fill[1*1024*1024*1024];
};
struct foo a;
void
main(void)
{
}

Quoted 4 lines

Compile. Run. Segfault.
Whose fault? Kernels; it fails to corectly report not enough address space.

I think the bug has to be found earlier on up the food chain. If you do:

char a[1*1024*1024*1024] = {0,};

int main(void);
int main()

{
    return 0;
}

The linker will run forever, un-killable...

      gcc -o xxx xxx.c 
     0     0 31478 31466   9   0    752   280 wait4       S    1  0:00
      /usr/local/lib/gcc-lib/i686-pc-linux-gnu/egcs-2.91.66/collect2
      -m elf_i386 -dynamic-linke
     0     0 31479 31478  14   0 1050176 255184 lock_page   D    1  0:23
      /usr/local/i686-pc-linux-gnu/bin/ld -m elf_i386 -dynamic-linker
      /lib/ld-linux.so.2 -o xxx

I can kill the top-level, but the task in lock_page runs forever, paging to swap-file like crazy.

The output file grew to 1 gb, then stopped.

Cheers,
Dick Johnson

Penguin : Linux version 2.4.18 on an i686 machine (799.53 BogoMips).

Bill Gates? Who?

Pavel Machek

2002-03-06 15:21 UTC #2 lore raw

In reply to Pavel Machek

Hi!

Quoted 14 lines

Quoted 14 lines
Take this trivial .c program. Obviously correct.
struct foo {
        char fill[1*1024*1024*1024];
};
struct foo a;
void
main(void)
{
}

Quoted 7 lines

Quoted 4 lines
Compile. Run. Segfault.
Whose fault? Kernels; it fails to corectly report not enough address space.
I think the bug has to be found earlier on up the food chain. If you do:

Well, nothing earlier can know if there will be enough address space at runtime. (It depends on kernel config .. 3GB vs 2GB ...)
Pavel

Signature · 2 lines

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Russell King

2002-03-15 13:02 UTC #3 lore raw

In reply to Pavel Machek

Quoted 2 lines · Pavel Machek

Now, I do not know if this is the right fix (binfmt_elf looks spaghetty to me) but its certainly better than it was.

Here's another fix in this area. If setup_arg_pages() fails, we continue although nothing went wrong. The following patch kills the process instead.

Linus/Marcelo - please apply.

fs/binfmt_elf.c +6 -1

@@ -585,7 +585,12 @@
 	/* Do this so that we can load the interpreter, if need be.  We will
 	   change some of these later */
 	current->mm->rss = 0;
-	setup_arg_pages(bprm); /* XXX: check error */
+	retval = setup_arg_pages(bprm);
+	if (retval < 0) {
+		send_sig(SIGKILL, current, 0);
+		return retval;
+	}
+	
 	current->mm->start_stack = bprm->p;
 
 	/* Now we do a little grungy work by mmaping the ELF image into

Signature · 2 lines

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html