[Patch 0/9] Fix resource leaks for exec related stuffs

[Patch 0/9] Fix resource leaks for exec related stuffs

[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.

Signed-off-by: WANG Cong <wangcong@zeuux.org>

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

[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().

Signed-off-by: WANG Cong <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>

---
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index b512e48..a2bfa57 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -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);
diff --git a/fs/exec.c b/fs/exec.c
index aeaa979..b49ba41 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:26PM +0800, WANG Cong wrote:
> 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.

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

[WANG Cong]

On Sat, May 10, 2008 at 08:12:51PM +0100, Al Viro wrote:
>On Thu, May 08, 2008 at 09:52:26PM +0800, WANG Cong wrote:
>> 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. ;-)

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

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

[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 <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>

---
diff --git a/fs/exec.c b/fs/exec.c
index aeaa979..b49ba41 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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);
 

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:27PM +0800, WANG Cong wrote:
> 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?

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

[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 <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>

---
diff --git a/fs/compat.c b/fs/compat.c
index 332a869..7e836ff 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -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);
 

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:28PM +0800, WANG Cong wrote:
> 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".

> @@ -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);
>  

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

[Al Viro]

On Sat, May 10, 2008 at 08:21:11PM +0100, Al Viro wrote:
> On Thu, May 08, 2008 at 09:52:28PM +0800, WANG Cong wrote:
> > 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:

diff --git a/fs/compat.c b/fs/compat.c
index 332a869..ed43e17 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -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;
diff --git a/fs/exec.c b/fs/exec.c
index aeaa979..ad545b0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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);
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index b512e48..c2f7fba 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -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 */

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

[WANG Cong]

On Sat, May 10, 2008 at 09:35:43PM +0100, Al Viro wrote:
>On Sat, May 10, 2008 at 08:21:11PM +0100, Al Viro wrote:
>> On Thu, May 08, 2008 at 09:52:28PM +0800, WANG Cong wrote:
>> > 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:


Hmm, looks fine. But, see below.

>
>diff --git a/fs/compat.c b/fs/compat.c
>index 332a869..ed43e17 100644
>--- a/fs/compat.c
>+++ b/fs/compat.c
>@@ -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;
>diff --git a/fs/exec.c b/fs/exec.c
>index aeaa979..ad545b0 100644
>--- a/fs/exec.c
>+++ b/fs/exec.c
>@@ -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);


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


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

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

[Al Viro]

On Mon, May 12, 2008 at 11:46:09AM +0800, WANG Cong wrote:
> >@@ -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);
> 
> 
> Why just drop this? No need to call free_bprm()?

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

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

[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 <wangcong@zeuux.org>

---
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 9e3963f..2c10342 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -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 = {

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:29PM +0800, WANG Cong wrote:
>  ->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?

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

[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 <wangcong@zeuux.org>

---
diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index f9c88d0..caa8466 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -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 = {

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:30PM +0800, WANG Cong wrote:
>  ->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.

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

[WANG Cong]

On Sat, May 10, 2008 at 08:25:09PM +0100, Al Viro wrote:
>On Thu, May 08, 2008 at 09:52:30PM +0800, WANG Cong wrote:
>>  ->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!

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

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

[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 <wangcong@zeuux.org>

---
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 7191306..bcc628c 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -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);

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:31PM +0800, WANG Cong wrote:
>  ->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.

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

[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 <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>

---
diff --git a/fs/exec.c b/fs/exec.c
index aeaa979..0237541 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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);

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:32PM +0800, WANG Cong wrote:
> 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?

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

[WANG Cong]

On Sat, May 10, 2008 at 08:31:05PM +0100, Al Viro wrote:
>On Thu, May 08, 2008 at 09:52:32PM +0800, WANG Cong wrote:
>> 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;
        }


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

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

[Al Viro]

On Mon, May 12, 2008 at 11:56:43AM +0800, WANG Cong wrote:
> On Sat, May 10, 2008 at 08:31:05PM +0100, Al Viro wrote:
> >On Thu, May 08, 2008 at 09:52:32PM +0800, WANG Cong wrote:
> >> 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...

>         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".

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

[WANG Cong]

On Mon, May 12, 2008 at 05:01:22AM +0100, Al Viro wrote:
>On Mon, May 12, 2008 at 11:56:43AM +0800, WANG Cong wrote:
>> On Sat, May 10, 2008 at 08:31:05PM +0100, Al Viro wrote:
>> >On Thu, May 08, 2008 at 09:52:32PM +0800, WANG Cong wrote:
>> >> 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...
>

Hmm, it should. Thanks for your correction.


>>         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.

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

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

[Al Viro]

On Mon, May 12, 2008 at 12:15:34PM +0800, WANG Cong wrote:
> 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.

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

[WANG Cong]

On Mon, May 12, 2008 at 05:37:58AM +0100, Al Viro wrote:
>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. :-)

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

[Patch 8/9] fs/binfmt_elf.c: fix wrong return values

[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 <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>

---
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 43254e3..10f9642 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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))

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

[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 <wangcong@zeuux.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>

---
diff --git a/fs/exec.c b/fs/exec.c
index aeaa979..0220427 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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) {

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

[Al Viro]

On Thu, May 08, 2008 at 09:52:34PM +0800, WANG Cong wrote:
> 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"?

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

[WANG Cong]

On Sat, May 10, 2008 at 08:37:01PM +0100, Al Viro wrote:
>On Thu, May 08, 2008 at 09:52:34PM +0800, WANG Cong wrote:
>> 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?

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

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

[Al Viro]

On Mon, May 12, 2008 at 11:51:51AM +0800, WANG Cong wrote:
> On Sat, May 10, 2008 at 08:37:01PM +0100, Al Viro wrote:
> >On Thu, May 08, 2008 at 09:52:34PM +0800, WANG Cong wrote:
> >> 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.

Re: [Patch 0/9] Fix resource leaks for exec related stuffs

[Al Viro]

On Thu, May 08, 2008 at 09:52:25PM +0800, WANG Cong wrote:
> 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.