Fix resource leaks for exec related stuffs

PATCH In review WANG Cong 2 participants 31 messages May 8, 2008 – May 12, 2008

Subsystems (2)

VFS exec and binfmt

lore

Share & tools

lore

WANG Cong

author

2008-05-08 13:54 UTC #0 lore raw

cover Fix resource leaks for exec related stuffs

This patchset contains various fixes for fs/binfmt_*.c, fs/{exec, compat}.c. Most of them are aimed to fix resource leaks in failure path. Others are bug fixes in failure path.

All patches are tested on UML, except [5/9] and [6/9], since I don't have the desired environment to test them. It should be OK since [4/9] is the similar fix.

Signed-off-by WANG Cong <[email protected]>

WANG Cong

author

2008-05-08 13:55 UTC #1 lore raw

PATCH 1/9 fs/exec.c: export free_arg_pages

free_arg_pages() is used to clean the pages allocated by copy_strings_kenrel(). Since copy_strings_kernel() is exported, so should be free_arg_pages().

Signed-off-by WANG Cong <[email protected]>

Cc Alexander Viro <[email protected]>

---

include/linux/binfmts.h +1 -0

index b512e48..a2bfa57 100644
@@ -96,6 +96,7 @@ extern int setup_arg_pages(struct linux_binprm * bprm,
 			   int executable_stack);
 extern int bprm_mm_init(struct linux_binprm *bprm);
 extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);
+extern void free_arg_pages(struct linux_binprm *bprm);
 extern void compute_creds(struct linux_binprm *binprm);
 extern int do_coredump(long signr, int exit_code, struct pt_regs * regs);
 extern int set_binfmt(struct linux_binfmt *new);

fs/exec.c +4 -2

index aeaa979..b49ba41 100644
@@ -210,7 +210,7 @@ static void free_arg_page(struct linux_binprm *bprm, int i)
 {
 }
 
-static void free_arg_pages(struct linux_binprm *bprm)
+void free_arg_pages(struct linux_binprm *bprm)
 {
 }

@@ -301,7 +301,7 @@ static void free_arg_page(struct linux_binprm *bprm, int i)
 	}
 }
 
-static void free_arg_pages(struct linux_binprm *bprm)
+void free_arg_pages(struct linux_binprm *bprm)
 {
 	int i;

@@ -327,6 +327,8 @@ static bool valid_arg_len(struct linux_binprm *bprm, long len)
 
 #endif /* CONFIG_MMU */
 
+EXPORT_SYMBOL(free_arg_pages);
+
 /*
  * Create a new mm_struct and populate it with a temporary stack
  * vm_area_struct.  We don't have enough context at this point to set the stack

Al Viro

maintainer

2008-05-10 19:13 UTC #2 lore raw

Re: [Patch 1/9] fs/exec.c: export free_arg_pages

Quoted 2 lines · WANG Cong

free_arg_pages() is used to clean the pages allocated by copy_strings_kenrel(). Since copy_strings_kernel() is exported, so should be free_arg_pages().

Not really. free_arg_pages() cleans them all at once; copy_strings_kernel() adds to pile. free_arg_pages() is called once for _all_ of them. And that happens in fs/exec.c, so it doesn't need to be exported.

WANG Cong

author

2008-05-12 04:02 UTC #3 lore raw

Re: [Patch 1/9] fs/exec.c: export free_arg_pages

Quoted 7 lines · Al Viro

Quoted 2 lines · WANG Cong
free_arg_pages() is used to clean the pages allocated by copy_strings_kenrel(). Since copy_strings_kernel() is exported, so should be free_arg_pages().
Not really. free_arg_pages() cleans them all at once; copy_strings_kernel() adds to pile. free_arg_pages() is called once for _all_ of them. And that happens in fs/exec.c, so it doesn't need to be exported.

Yes, thanks.
I already saw your patch, looks better than mine. ;-)

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

WANG Cong

author

2008-05-08 13:55 UTC #4 lore raw

PATCH 2/9 fs/exec.c: fix resource leaks and wrong goto's

In reply to WANG Cong

When ->load_binary() successed, free_arg_pages() should be called to clean the pages allocated by copy_strings_kernel() within it.

And also fixes some wrong goto pathes.

Signed-off-by WANG Cong <[email protected]>

Cc Alexander Viro <[email protected]>

---

fs/exec.c +6 -3

index aeaa979..b49ba41 100644
@@ -1218,6 +1220,7 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 				if (bprm->file)
 					fput(bprm->file);
 				bprm->file = NULL;
+				free_arg_pages(bprm);
 				current->did_exec = 1;
 				proc_exec_connector(current);
 				return retval;

@@ -1298,15 +1301,15 @@ int do_execve(char * filename,
 
 	retval = security_bprm_alloc(bprm);
 	if (retval)
-		goto out;
+		goto out_mm;
 
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		goto out;
+		goto out_sec;
 
 	retval = copy_strings_kernel(1, &bprm->filename, bprm);
 	if (retval < 0)
-		goto out;
+		goto out_sec;
 
 	bprm->exec = bprm->p;
 	retval = copy_strings(bprm->envc, envp, bprm);

@@ -1331,6 +1334,8 @@ int do_execve(char * filename,
 
 out:
 	free_arg_pages(bprm);
+
+out_sec:
 	if (bprm->security)
 		security_bprm_free(bprm);

Al Viro

maintainer

2008-05-10 19:17 UTC #5 lore raw

Re: [Patch 2/9] fs/exec.c: fix resource leaks and wrong goto's

Quoted 2 lines · WANG Cong

When ->load_binary() successed, free_arg_pages() should be called to clean the pages allocated by copy_strings_kernel() within it.

Eh? Success or not, free_arg_pages() will eventually be called by do_execve(); so what problem are you trying to solve?

WANG Cong

author

2008-05-08 13:56 UTC #6 lore raw

PATCH 3/9 fs/compat.c: fix resource leaks and wrong goto's

In reply to WANG Cong

Use free_arg_pages() to free the pages allocated by copy_strings_kernel() on failure. And fix some related wrong goto pathes.

Signed-off-by WANG Cong <[email protected]>

Cc Alexander Viro <[email protected]>

---

fs/compat.c +7 -3

index 332a869..7e836ff 100644
@@ -1381,15 +1381,15 @@ int compat_do_execve(char * filename,
 
 	retval = security_bprm_alloc(bprm);
 	if (retval)
-		goto out;
+		goto out_mm;
 
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		goto out;
+		goto out_sec;
 
 	retval = copy_strings_kernel(1, &bprm->filename, bprm);
 	if (retval < 0)
-		goto out;
+		goto out_sec;
 
 	bprm->exec = bprm->p;
 	retval = compat_copy_strings(bprm->envc, envp, bprm);

@@ -1403,6 +1403,7 @@ int compat_do_execve(char * filename,
 	retval = search_binary_handler(bprm, regs);
 	if (retval >= 0) {
 		/* execve success */
+		free_arg_pages(bprm);
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
 		kfree(bprm);

@@ -1410,6 +1411,9 @@ int compat_do_execve(char * filename,
 	}
 
 out:
+	free_arg_pages(bprm);
+
+out_sec:
 	if (bprm->security)
 		security_bprm_free(bprm);

Al Viro

maintainer

2008-05-10 19:21 UTC #7 lore raw

Re: [Patch 3/9] fs/compat.c: fix resource leaks and wrong goto's

Quoted 2 lines · WANG Cong

Use free_arg_pages() to free the pages allocated by copy_strings_kernel() on failure. And fix some related wrong goto pathes.

Kinda; free_arg_pages() is needed here, but there's no need to mess with goto - just put it under out:

Note that it's essentially just a part of freeing bprm; it's _not_ a "clean collected strings, so that we could add new ones".

Quoted 18 lines

fs/compat.c +4 -0

@@ -1403,6 +1403,7 @@ int compat_do_execve(char * filename,
 	retval = search_binary_handler(bprm, regs);
 	if (retval >= 0) {
 		/* execve success */
+		free_arg_pages(bprm);
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
 		kfree(bprm);

@@ -1410,6 +1411,9 @@ int compat_do_execve(char * filename,
 	}
 
 out:
+	free_arg_pages(bprm);
+
+out_sec:
 	if (bprm->security)
 		security_bprm_free(bprm);

Al Viro

maintainer

2008-05-10 20:35 UTC #8 lore raw

Re: [Patch 3/9] fs/compat.c: fix resource leaks and wrong goto's

Quoted 9 lines · Al Viro

Quoted 2 lines · WANG Cong
Use free_arg_pages() to free the pages allocated by copy_strings_kernel() on failure. And fix some related wrong goto pathes.
Kinda; free_arg_pages() is needed here, but there's no need to mess with goto - just put it under out:
Note that it's essentially just a part of freeing bprm; it's _not_ a "clean collected strings, so that we could add new ones".

FWIW, I'd suggest this:

fs/compat.c +2 -2

index 332a869..ed43e17 100644
@@ -1405,7 +1405,7 @@ int compat_do_execve(char * filename,
 		/* execve success */
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
-		kfree(bprm);
+		free_bprm(bprm);
 		return retval;
 	}

@@ -1424,7 +1424,7 @@ out_file:
 	}
 
 out_kfree:
-	kfree(bprm);
+	free_bprm(bprm);
 
 out_ret:
 	return retval;

fs/exec.c +7 -3

index aeaa979..ad545b0 100644
@@ -1251,6 +1251,12 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 
 EXPORT_SYMBOL(search_binary_handler);
 
+void free_bprm(struct linux_binprm *bprm)
+{
+	free_arg_pages(bprm);
+	kfree(bprm);
+}
+
 /*
  * sys_execve() executes a new program.
  */

@@ -1320,17 +1326,15 @@ int do_execve(char * filename,
 	retval = search_binary_handler(bprm,regs);
 	if (retval >= 0) {
 		/* execve success */
-		free_arg_pages(bprm);
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
-		kfree(bprm);
+		free_bprm(bprm);
 		if (displaced)
 			put_files_struct(displaced);
 		return retval;
 	}
 
 out:
-	free_arg_pages(bprm);
 	if (bprm->security)
 		security_bprm_free(bprm);

include/linux/binfmts.h +1 -0

index b512e48..c2f7fba 100644
@@ -99,6 +99,7 @@ extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);
 extern void compute_creds(struct linux_binprm *binprm);
 extern int do_coredump(long signr, int exit_code, struct pt_regs * regs);
 extern int set_binfmt(struct linux_binfmt *new);
+extern void free_bprm(struct linux_binfmt *);
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_BINFMTS_H */

WANG Cong

author

2008-05-12 03:48 UTC #9 lore raw

Re: [Patch 3/9] fs/compat.c: fix resource leaks and wrong goto's

Quoted 5 lines · Al Viro

Quoted 4 lines · Al Viro
Quoted 2 lines · WANG Cong
Use free_arg_pages() to free the pages allocated by copy_strings_kernel() on failure. And fix some related wrong goto pathes.

Quoted 7 lines

Quoted 5 lines
Kinda; free_arg_pages() is needed here, but there's no need to mess with goto - just put it under out:
Note that it's essentially just a part of freeing bprm; it's _not_ a "clean collected strings, so that we could add new ones".
FWIW, I'd suggest this:

Hmm, looks fine. But, see below.

Quoted 55 lines

fs/compat.c +1 -1

index 332a869..ed43e17 100644
@@ -1405,7 +1405,7 @@ int compat_do_execve(char * filename,
 		/* execve success */
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
-		kfree(bprm);
+		free_bprm(bprm);
 		return retval;
 	}

@@ -1424,7 +1424,7 @@ out_file:
 	}

out_kfree:

fs/compat.c +1 -1

-	kfree(bprm);
+	free_bprm(bprm);

out_ret:

	return retval;

fs/exec.c +7 -2

index aeaa979..ad545b0 100644
@@ -1251,6 +1251,12 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 
 EXPORT_SYMBOL(search_binary_handler);
 
+void free_bprm(struct linux_binprm *bprm)
+{
+	free_arg_pages(bprm);
+	kfree(bprm);
+}
+
 /*
 * sys_execve() executes a new program.
 */

@@ -1320,17 +1326,15 @@ int do_execve(char * filename,
 	retval = search_binary_handler(bprm,regs);
 	if (retval >= 0) {
 		/* execve success */
-		free_arg_pages(bprm);
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
-		kfree(bprm);
+		free_bprm(bprm);
 		if (displaced)
 			put_files_struct(displaced);
 		return retval;
 	}

out:

fs/exec.c

Quoted 1 line

fs/exec.c +0 -1
-	free_arg_pages(bprm);

Why just drop this? No need to call free_bprm()?

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

Al Viro

maintainer

2008-05-12 04:06 UTC #10 lore raw

Re: [Patch 3/9] fs/compat.c: fix resource leaks and wrong goto's

Quoted 14 lines · WANG Cong

Quoted 14 lines

fs/exec.c +1 -2

@@ -1320,17 +1326,15 @@ int do_execve(char * filename,
 	retval = search_binary_handler(bprm,regs);
 	if (retval >= 0) {
 		/* execve success */
-		free_arg_pages(bprm);
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
-		kfree(bprm);
+		free_bprm(bprm);
 		if (displaced)
 			put_files_struct(displaced);
 		return retval;
 	}

Quoted 5 lines

Review
Quoted 2 lines
out:
(quoted diff) +0 -1
-	free_arg_pages(bprm);
Why just drop this? No need to call free_bprm()?

Fixed, pushed to vfs-2.6.git...

WANG Cong

author

2008-05-08 13:56 UTC #11 lore raw

PATCH 4/9 fs/binfmt_script.c: fix resource leaks

In reply to WANG Cong

->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.

Signed-off-by WANG Cong <[email protected]>

---

fs/binfmt_script.c +20 -6

index 9e3963f..2c10342 100644
@@ -75,11 +75,13 @@ static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
 	bprm->argc++;
 	if (i_arg) {
 		retval = copy_strings_kernel(1, &i_arg, bprm);
-		if (retval < 0) return retval; 
+		if (retval < 0)
+			goto out;
 		bprm->argc++;
 	}
 	retval = copy_strings_kernel(1, &i_name, bprm);
-	if (retval) return retval; 
+	if (retval)
+		goto out;
 	bprm->argc++;
 	bprm->interp = interp;

@@ -87,14 +89,26 @@ static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
 	 * OK, now restart the process with the interpreter's dentry.
 	 */
 	file = open_exec(interp);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
+	if (IS_ERR(file)) {
+		retval = PTR_ERR(file);
+		goto out;
+	}
 
 	bprm->file = file;
 	retval = prepare_binprm(bprm);
-	if (retval < 0)
-		return retval;
+	if (retval < 0) {
+		if (bprm->file) {
+			allow_write_access(bprm->file);
+			fput(bprm->file);
+		}
+		goto out;
+	}
+
 	return search_binary_handler(bprm,regs);
+
+out:
+	free_arg_pages(bprm);
+	return retval;
 }
 
 static struct linux_binfmt script_format = {

Al Viro

maintainer

2008-05-10 19:24 UTC #12 lore raw

Re: [Patch 4/9] fs/binfmt_script.c: fix resource leaks

Quoted 3 lines · WANG Cong

->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.

Huh??? Just what does the end of do_execve() do in your opinion?

WANG Cong

author

2008-05-08 13:57 UTC #13 lore raw

PATCH 5/9 fs/binfmt_em86.c: fix resource leaks

In reply to WANG Cong

->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.

Signed-off-by WANG Cong <[email protected]>

---

fs/binfmt_em86.c +19 -6

index f9c88d0..caa8466 100644
@@ -69,11 +69,13 @@ static int load_em86(struct linux_binprm *bprm,struct pt_regs *regs)
 	bprm->argc++;
 	if (i_arg) {
 		retval = copy_strings_kernel(1, &i_arg, bprm);
-		if (retval < 0) return retval; 
+		if (retval < 0)
+			goto out;
 		bprm->argc++;
 	}
 	retval = copy_strings_kernel(1, &i_name, bprm);
-	if (retval < 0)	return retval;
+	if (retval < 0)
+		goto out;
 	bprm->argc++;
 
 	/*

@@ -82,16 +84,27 @@ static int load_em86(struct linux_binprm *bprm,struct pt_regs *regs)
 	 * space, and we don't need to copy it.
 	 */
 	file = open_exec(interp);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
+	if (IS_ERR(file)) {
+		retval = PTR_ERR(file);
+		goto out;
+	}
 
 	bprm->file = file;
 
 	retval = prepare_binprm(bprm);
-	if (retval < 0)
-		return retval;
+	if (retval < 0) {
+		if (bprm->file) {
+			allow_write_access(bprm->file);
+			fput(bprm->file);
+		}
+		goto out;
+	}
 
 	return search_binary_handler(bprm, regs);
+
+out:
+	free_arg_pages(bprm);
+	return retval;
 }
 
 static struct linux_binfmt em86_format = {

Al Viro

maintainer

2008-05-10 19:25 UTC #14 lore raw

Re: [Patch 5/9] fs/binfmt_em86.c: fix resource leaks

Quoted 3 lines · WANG Cong

->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.

No, it should not. do_execve() will free them just fine.

WANG Cong

author

2008-05-12 03:55 UTC #15 lore raw

Re: [Patch 5/9] fs/binfmt_em86.c: fix resource leaks

Quoted 6 lines · Al Viro

Quoted 3 lines · WANG Cong
->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.
No, it should not. do_execve() will free them just fine.

Oh, yes. I missed this. So 4,5,6 should be dropped.

Thanks!

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

WANG Cong

author

2008-05-08 13:57 UTC #16 lore raw

PATCH 6/9 fs/binfmt_misc.c: fix resource leaks

In reply to WANG Cong

->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.

Signed-off-by WANG Cong <[email protected]>

---

fs/binfmt_misc.c +12 -4

index 7191306..bcc628c 100644
@@ -174,7 +174,7 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 	/* add the interp as argv[0] */
 	retval = copy_strings_kernel (1, &iname_addr, bprm);
 	if (retval < 0)
-		goto _error;
+		goto _error_free;
 	bprm->argc ++;
 
 	bprm->interp = iname;	/* for binfmt_script */

@@ -182,7 +182,7 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 	interp_file = open_exec (iname);
 	retval = PTR_ERR (interp_file);
 	if (IS_ERR (interp_file))
-		goto _error;
+		goto _error_free;
 
 	bprm->file = interp_file;
 	if (fmt->flags & MISC_FMT_CREDENTIALS) {

@@ -196,14 +196,22 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 		retval = prepare_binprm (bprm);
 
 	if (retval < 0)
-		goto _error;
+		goto _error_file;
 
 	retval = search_binary_handler (bprm, regs);
 	if (retval < 0)
-		goto _error;
+		goto _error_file;
 
 _ret:
 	return retval;
+
+_error_file:
+	allow_write_access(bprm->file);
+	fput(bprm->file);
+
+_error_free:
+	free_arg_pages(bprm);
+
 _error:
 	if (fd_binary > 0)
 		sys_close(fd_binary);

Al Viro

maintainer

2008-05-10 19:25 UTC #17 lore raw

Re: [Patch 6/9] fs/binfmt_misc.c: fix resource leaks

Quoted 3 lines · WANG Cong

->load_binary()'s caller only frees resources which ->load_binary() applied
when it succeeded, so ->load_binary() itself should free the resources on failure.

Same problem.

WANG Cong

author

2008-05-08 13:57 UTC #18 lore raw

PATCH 7/9 fs/exec.c: fix wrong return value of prepare_binprm()

In reply to WANG Cong

All prepare_binprm()'s callers assume that prepare_binprm() fails when it returns negative. However, prepare_binprm() most probably returns the return value of kernel_read(), which may return positive on failure!

Thus this should be fixed.

Signed-off-by WANG Cong <[email protected]>

Cc Alexander Viro <[email protected]>

---

fs/exec.c +6 -1

index aeaa979..0237541 100644
@@ -1063,7 +1063,11 @@ int prepare_binprm(struct linux_binprm *bprm)
 		return retval;
 
 	memset(bprm->buf,0,BINPRM_BUF_SIZE);
-	return kernel_read(bprm->file,0,bprm->buf,BINPRM_BUF_SIZE);
+	retval = kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE);
+	if (retval < 0)
+		return retval;
+	if (retval != BINPRM_BUF_SIZE && retval != inode->i_size)
+		return -EIO;
+	return 0;
 }

EXPORT_SYMBOL(prepare_binprm);

Al Viro

maintainer

2008-05-10 19:31 UTC #19 lore raw

Re: [Patch 7/9] fs/exec.c: fix wrong return value of prepare_binprm()

Quoted 5 lines · WANG Cong

All prepare_binprm()'s callers assume that prepare_binprm() fails when it returns negative. However, prepare_binprm() most probably returns the return value of kernel_read(), which may return positive on failure!
Thus this should be fixed.

Since when does read return positive on failure?

WANG Cong

author

2008-05-12 03:58 UTC #20 lore raw

Re: [Patch 7/9] fs/exec.c: fix wrong return value of prepare_binprm()

Quoted 8 lines · Al Viro

Quoted 5 lines · WANG Cong
All prepare_binprm()'s callers assume that prepare_binprm() fails when it returns negative. However, prepare_binprm() most probably returns the return value of kernel_read(), which may return positive on failure!
Thus this should be fixed.
Since when does read return positive on failure?

When an EIO occurs, I think. For example,

        retval = kernel_read(interpreter, interp_elf_ex->e_phoff,
                             (char *)elf_phdata,size);
        error = -EIO;
        if (retval != size) {
                if (retval < 0)
                        error = retval;
                goto out_close;
        }

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

Al Viro

maintainer

2008-05-12 04:01 UTC #21 lore raw

Re: [Patch 7/9] fs/exec.c: fix wrong return value of prepare_binprm()

Quoted 11 lines · WANG Cong

Quoted 8 lines · Al Viro
Quoted 5 lines · WANG Cong
All prepare_binprm()'s callers assume that prepare_binprm() fails when it returns negative. However, prepare_binprm() most probably returns the return value of kernel_read(), which may return positive on failure!
Thus this should be fixed.
Since when does read return positive on failure?
When an EIO occurs, I think. For example,

No. On EIO it returns -EIO, TYVM...

Quoted 8 lines

        retval = kernel_read(interpreter, interp_elf_ex->e_phoff,
                             (char *)elf_phdata,size);
        error = -EIO;
        if (retval != size) {
                if (retval < 0)
                        error = retval;
                goto out_close;
        }

Which is what we do on short read here. FWIW, -EINVAL might be saner choice - it's "binary is corrupted", not "read had failed".

WANG Cong

author

2008-05-12 04:17 UTC #22 lore raw

Re: [Patch 7/9] fs/exec.c: fix wrong return value of prepare_binprm()

Quoted 7 lines · Al Viro

Quoted 6 lines · WANG Cong
Quoted 5 lines · Al Viro
Quoted 4 lines · WANG Cong
All prepare_binprm()'s callers assume that prepare_binprm() fails when it returns negative. However, prepare_binprm() most probably returns the return value of kernel_read(), which may return positive on failure!

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Thus this should be fixed.
Since when does read return positive on failure?
When an EIO occurs, I think. For example,
No. On EIO it returns -EIO, TYVM...

Hmm, it should. Thanks for your correction.

Quoted 11 lines

Quoted 8 lines
        retval = kernel_read(interpreter, interp_elf_ex->e_phoff,
                             (char *)elf_phdata,size);
        error = -EIO;
        if (retval != size) {
                if (retval < 0)
                        error = retval;
                goto out_close;
        }
Which is what we do on short read here. FWIW, -EINVAL might be saner choice - it's "binary is corrupted", not "read had failed".

IMO, -EIO is fine, because -EINVAL means "Invalid argument", but there's nothing wrong with arguments here, just some bad things occurred on reading the binary.

And even if it is really "binary is corrupted", then -ENOEXEC is better than -EINVAL, isn't it?

Anyway, kernel_read() may return postive when not success.

Thanks.

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

Al Viro

maintainer

2008-05-12 04:38 UTC #23 lore raw

Re: [Patch 7/9] fs/exec.c: fix wrong return value of prepare_binprm()

Quoted 4 lines · WANG Cong

And even if it is really "binary is corrupted", then -ENOEXEC is better than -EINVAL, isn't it?
Anyway, kernel_read() may return postive when not success.

???

It returns positives *exactly* on success. In the case you've quoted we don't have any problems with read; we do have a problem with _short_ read (i.e. miscalculated field size or truncated binary). In the case you've patched we _expect_ a short read; it's normal for short scripts, to start with. And we are ready to deal with it - the buffer is prefilled with zeroes and either we have enough to recognize signature (in which case we'll find the binfmt handler and let it deal with the entire thing, with full checks of its own) or we will not, in which case nobody will recognize the damn thing and that's it.

WANG Cong

author

2008-05-12 04:54 UTC #24 lore raw

Re: [Patch 7/9] fs/exec.c: fix wrong return value of prepare_binprm()

Quoted 9 lines · Al Viro

It returns positives *exactly* on success. In the case you've quoted we don't have any problems with read; we do have a problem with _short_ read (i.e. miscalculated field size or truncated binary). In the case you've patched we _expect_ a short read; it's normal for short scripts, to start with. And we are ready to deal with it - the buffer is prefilled with zeroes and either we have enough to recognize signature (in which case we'll find the binfmt handler and let it deal with the entire thing, with full checks of its own) or we will not, in which case nobody will recognize the damn thing and that's it.

Thanks for your detailed explanation.

Since it's expected, it's needless to fix it. :-)

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

WANG Cong

author

2008-05-08 13:58 UTC #25 lore raw

PATCH 8/9 fs/binfmt_elf.c: fix wrong return values

In reply to WANG Cong

create_elf_tables() returns 0 on success. But when strnlen_user() "fails", it returns 0 directly. So this is wrong.

Signed-off-by WANG Cong <[email protected]>

Cc Alexander Viro <[email protected]>

---

fs/binfmt_elf.c +2 -2

index 43254e3..10f9642 100644
@@ -256,7 +256,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 			return -EFAULT;
 		len = strnlen_user((void __user *)p, MAX_ARG_STRLEN);
 		if (!len || len > MAX_ARG_STRLEN)
-			return 0;
+			return -EINVAL;
 		p += len;
 	}
 	if (__put_user(0, argv))

@@ -268,7 +268,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 			return -EFAULT;
 		len = strnlen_user((void __user *)p, MAX_ARG_STRLEN);
 		if (!len || len > MAX_ARG_STRLEN)
-			return 0;
+			return -EINVAL;
 		p += len;
 	}
 	if (__put_user(0, envp))

WANG Cong

author

2008-05-08 13:58 UTC #26 lore raw

PATCH 9/9 fs/exec.c: fix a wrong goto path

In reply to WANG Cong

When nameidata_to_filp() fails, I see no reasons to fall into 'out' path where doesn't free any resources at all.

Signed-off-by WANG Cong <[email protected]>

Cc Alexander Viro <[email protected]>

---

fs/exec.c +1 -1

index aeaa979..0220427 100644
@@ -123,7 +123,7 @@ asmlinkage long sys_uselib(const char __user * library)
 	file = nameidata_to_filp(&nd, O_RDONLY|O_LARGEFILE);
 	error = PTR_ERR(file);
 	if (IS_ERR(file))
-		goto out;
+		goto exit;
 
 	error = -ENOEXEC;
 	if(file->f_op) {

Al Viro

maintainer

2008-05-10 19:37 UTC #27 lore raw

Re: [Patch 9/9] fs/exec.c: fix a wrong goto path

Quoted 2 lines · WANG Cong

When nameidata_to_filp() fails, I see no reasons to fall into 'out' path where doesn't free any resources at all.

How about "nameidata_to_filp() frees on failure"?

WANG Cong

author

2008-05-12 03:54 UTC #28 lore raw

Re: [Patch 9/9] fs/exec.c: fix a wrong goto path

Quoted 5 lines · Al Viro

Quoted 2 lines · WANG Cong
When nameidata_to_filp() fails, I see no reasons to fall into 'out' path where doesn't free any resources at all.
How about "nameidata_to_filp() frees on failure"?

When it fails, it should free the resources that were allocated before it. That's to say, it should goto 'exit' on failure.

Am I missing something obvious?

Signature · 2 lines

Hi, I'm a .signature virus, please copy/paste me to help me spread
all over the world.

Al Viro

maintainer

2008-05-12 03:58 UTC #29 lore raw

Re: [Patch 9/9] fs/exec.c: fix a wrong goto path

Quoted 11 lines · WANG Cong

Quoted 5 lines · Al Viro
Quoted 2 lines · WANG Cong
When nameidata_to_filp() fails, I see no reasons to fall into 'out' path where doesn't free any resources at all.
How about "nameidata_to_filp() frees on failure"?
When it fails, it should free the resources that were allocated before it. That's to say, it should goto 'exit' on failure.
Am I missing something obvious?

Yes. Take a look at the things dealt with at exit. Then look at what nameidata_to_filp() does on failure.

Al Viro

maintainer

2008-05-10 20:47 UTC #30 lore raw

In reply to WANG Cong

Quoted 7 lines · WANG Cong

This patchset contains various fixes for fs/binfmt_*.c, fs/{exec, compat}.c. Most of them are aimed to fix resource leaks in failure path. Others are bug fixes in failure path.
All patches are tested on UML, except [5/9] and [6/9], since I don't have the desired environment to test them. It should be OK since [4/9] is the similar fix.

3/9 and 8/9 are OK, the rest is either not needed or broken (e.g. 4/9 ends up with double fput() in case of failure in prepare_binfmt(), etc.)

I've applied "fs/binfmt_elf.c: fix a wrong free", slightly simple fix for lead covered by 3/9, and 8/9 as-is; all in vfs-2.6.git#vfs-2.6.25.