Search for patch for kernel stack disclosure in binfmt_script during execve

Search for patch for kernel stack disclosure in binfmt_script during execve

[halfdog]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I'm searching for a patch for linux kernel stack disclosure in
binfmt_script with crafted interpreter names when CONFIG_MODULES is
active (see [1]).

The simplest solution would be to return an error in load_script (from
fs/binfmt_script.c). when maximal recursion depth is reached, but I'm
not sure, if that is nice and could have any side effects. Apart from
that, some change in the loop condition in search_binary_handler (from
fs/exec.c) could have side effects hard to see and hence reintroduce
the bug (challenge to get that right in documentation).


Any comments?

- --- fs/binfmt_script.c  2012-01-19 23:04:48.000000000 +0000
+++ fs/binfmt_script.c        2012-08-18 13:55:25.735748407 +0000
@@ -22,9 +22,8 @@
        char interp[BINPRM_BUF_SIZE];
        int retval;

- -       if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
- -           (bprm->recursion_depth > BINPRM_MAX_RECURSION))
- -               return -ENOEXEC;
+       if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!')) return
- -ENOEXEC;
+        if (bprm->recursion_depth > BINPRM_MAX_RECURSION) return -ENOMEM;
        /*
         * This section does the #! interpretation.
         * Sorta complicated, but hopefully it will work.  -TYT

hd

[1]
http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/

- -- 
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAlAvn0MACgkQxFmThv7tq+6nUACfdk7KWESuC6J1FXZcrMaa3kCb
eWoAn0wV6INdYGjAZydd6ytO0i5BnhGa
=cxbR
-----END PGP SIGNATURE-----

Re: Search for patch for kernel stack data disclosure in binfmt_script during execve

[halfdog]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

halfdog wrote:
> I'm searching for a patch for linux kernel stack disclosure in 
> binfmt_script with crafted interpreter names when CONFIG_MODULES
> is active (see [1]).

Please disregard my previous proposal [2], since it did not address
the problem directly (referencing local stack frame data from bprm
structure) but worked around it. I suspect, that this could increase
probability to reintroduce similar bugs.

Opinions on (untested sketch for) second solution: Could someone look
on the source code comments and changes in patch to judge, if this is
going in the right direction?


Explanation of patch: Since load_script will start to irreversibly
change bprm structures at some point (using stack local data was one
of those changes), try to delay this point. Run checks if load_script
could be the right handler, if not give other binfmt handlers the
chance to do so.

If binfmt_script is the right one, try to load the interpreter
(causing bprm modification), if failing make sure that no other binfmt
handler has the chance to continue on the now modified bprm data.

CAVEAT: This assumes, that if binfmt_script could handle the load,
that it would be the one and only binfmt with that ability, so no
other one, e.g. binfmt_misc should have the chance to do so. If this
assumption is wrong, leaving binfmt_script would have to rollback all
bprm changes (e.g. restore old credentials).

hd

[1]
http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
[2] http://lkml.org/lkml/2012/8/18/75

- -- 
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAlAwphsACgkQxFmThv7tq+6UAQCgh7IA8UcqNieV41YKHS5/YxGE
IbcAn1uP1nIakg/gD1KlV0KNnLIfitEp
=5Klt
-----END PGP SIGNATURE-----

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5609 bytes --]

--- fs/binfmt_script.c	2012-01-19 23:04:48.000000000 +0000
+++ fs/binfmt_script.c	2012-08-19 07:08:42.540611605 +0000
@@ -14,12 +14,24 @@
 #include <linux/err.h>
 #include <linux/fs.h>
 
+/** Check if this handler is suitable to load the "binary" identified
+ *  by first BINPRM_BUF_SIZE bytes in bprm->buf.
+ *  @returns -ENOEXEC if this handler is not suitable for that type
+ *  of binary. In that case, the handler must not modify any of the
+ *  data associated with bprm.
+ *  Any error if the binary should have been handled by this loader
+ *  but handling failed. In that case. FIXME: be defensive? also
+ *  kill bprm->mm or bprm->file also to make it impossible, that
+ *  upper search_binary_handler can continue handling?
+ *  0 (OK) otherwise, the new executable is ready in bprm->mm.
+ */
 static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
 {
 	const char *i_arg, *i_name;
 	char *cp;
 	struct file *file;
-	char interp[BINPRM_BUF_SIZE];
+	char bprm_buf_copy[BINPRM_BUF_SIZE];
+	char *bprm_old_interp_name;
 	int retval;
 
 	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
@@ -30,25 +42,29 @@ static int load_script(struct linux_binp
 	 * Sorta complicated, but hopefully it will work.  -TYT
 	 */
 
-	bprm->recursion_depth++;
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+	/* Keep bprm unchanged until we known, that this is a script
+	 * to be handled by this loader. Copy bprm->buf for sure,
+	 * otherwise returning -ENOEXEC will make other handlers see
+	 * modified data. (hd)
+	 */
+	memcpy(bprm_buf_copy, bprm->buf, BINPRM_BUF_SIZE);
 
-	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-	if ((cp = strchr(bprm->buf, '\n')) == NULL)
-		cp = bprm->buf+BINPRM_BUF_SIZE-1;
+	bprm_buf_copy[BINPRM_BUF_SIZE - 1]='\0';
+	if ((cp = strchr(bprm_buf_copy, '\n')) == NULL)
+		cp = bprm_buf_copy+BINPRM_BUF_SIZE-1;
 	*cp = '\0';
-	while (cp > bprm->buf) {
+	while (cp > bprm_buf_copy) {
 		cp--;
 		if ((*cp == ' ') || (*cp == '\t'))
 			*cp = '\0';
 		else
 			break;
 	}
-	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
+	for (cp = bprm_buf_copy+2; (*cp == ' ') || (*cp == '\t'); cp++);
 	if (*cp == '\0') 
-		return -ENOEXEC; /* No interpreter name found */
+	/* No interpreter name found. No problem to let other handlers
+	 * retry, we did not change anything. */
+		return -ENOEXEC;
 	i_name = cp;
 	i_arg = NULL;
 	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
@@ -57,45 +73,83 @@ static int load_script(struct linux_binp
 		*cp++ = '\0';
 	if (*cp)
 		i_arg = cp;
-	strcpy (interp, i_name);
+
+	/* So this is our point-of-no-return: modification of bprm
+	 * will be irreversible, so if we fail to setup execution
+	 * using the new interpreter name (i_name), we have to make
+	 * sure, that no other handler tries again. (hd)
+	 */
+
 	/*
 	 * OK, we've parsed out the interpreter name and
 	 * (optional) argument.
 	 * Splice in (1) the interpreter's name for argv[0]
-	 *           (2) (optional) argument to interpreter
-	 *           (3) filename of shell script (replace argv[0])
+	 *	   (2) (optional) argument to interpreter
+	 *	   (3) filename of shell script (replace argv[0])
 	 *
 	 * This is done in reverse order, because of how the
 	 * user environment and arguments are stored.
 	 */
+
+	/* Ugly: we store pointer to local stack frame in bprm,
+	 * so make sure to clear this up before returning.
+	 */
+	bprm_old_interp_name = bprm->interp;
+	bprm->interp = i_name;
+
 	retval = remove_arg_zero(bprm);
-	if (retval)
-		return retval;
-	retval = copy_strings_kernel(1, &bprm->interp, bprm);
-	if (retval < 0) return retval; 
+	if (retval) goto out;
+	/* copy_strings_kernel is ok here, even when racy: since no
+	 * user can be attached to new mm, there is nobody to race
+	 * with and call is safe for now. And copy_strings_kernel
+	 * cannot return -ENOEXEC in any case. (hd)
+	 */
+	retval = copy_strings_kernel(1, &bprm_old_interp_name, bprm);
+	if (retval < 0) goto out;
 	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; 
+	retval = copy_strings_kernel(1, &bprm->interp, bprm);
+	if (retval) goto out;
 	bprm->argc++;
-	bprm->interp = interp;
 
 	/*
 	 * OK, now restart the process with the interpreter's dentry.
+         * Release old file first
 	 */
-	file = open_exec(interp);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
-
+	allow_write_access(bprm->file);
+	fput(bprm->file);
+	bprm->file = NULL;
+	file = open_exec(bprm->interp);
+	if (IS_ERR(file)) {
+		retval=PTR_ERR(file);
+		goto out;
+        }
 	bprm->file = file;
+	/* Caveat: This also updates the credentials of the next exec. */
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		return retval;
-	return search_binary_handler(bprm,regs);
+		goto out;
+	bprm->recursion_depth++;
+	retval=search_binary_handler(bprm,regs);
+
+out:	/* Make sure, we do not return local stack frame data. If
+	 * it would be needed after returning, we would have needed
+	 * to allocate memory or use copy from new bprm->mm anyway. (hd)
+         */
+	bprm->interp = bprm_old_interp_name; 
+	if(!retval) {
+		/* The handlers for starting of interpreter failed.
+		 * bprm is already modified, hence we are dead here.
+		 * Make sure, that we do not return -ENOEXEC, that would
+		 * allow searching for handlers to continue. (hd).
+		 */
+		if(retval==-ENOEXEC) retval=-EINVAL;
+	}
+	return(retval);
 }
 
 static struct linux_binfmt script_format = {

Re: Search for patch for kernel stack data disclosure in binfmt_script during execve

[halfdog]

Got a hint via IRC, that I should not send patch idea for review to
"generic" list, but to maintainers and last (or relevant) comitters of code.

http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=commitdiff;h=bf2a9a39639b8b51377905397a5005f444e9a892

CC to generic just for the records

halfdog wrote:
> halfdog wrote:
>> I'm searching for a patch for linux kernel stack disclosure in 
>> binfmt_script with crafted interpreter names when CONFIG_MODULES
>> is active (see [1]).
> 
> Please disregard my previous proposal [2], since it did not address
> the problem directly (referencing local stack frame data from bprm
> structure) but worked around it. I suspect, that this could increase
> probability to reintroduce similar bugs.
> 
> Opinions on (untested sketch for) second solution: Could someone look
> on the source code comments and changes in patch to judge, if this is
> going in the right direction?
> 
> 
> Explanation of patch: Since load_script will start to irreversibly
> change bprm structures at some point (using stack local data was one
> of those changes), try to delay this point. Run checks if load_script
> could be the right handler, if not give other binfmt handlers the
> chance to do so.
> 
> If binfmt_script is the right one, try to load the interpreter
> (causing bprm modification), if failing make sure that no other binfmt
> handler has the chance to continue on the now modified bprm data.
> 
> CAVEAT: This assumes, that if binfmt_script could handle the load,
> that it would be the one and only binfmt with that ability, so no
> other one, e.g. binfmt_misc should have the chance to do so. If this
> assumption is wrong, leaving binfmt_script would have to rollback all
> bprm changes (e.g. restore old credentials).
> 
> hd
> 
> [1]
> http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
> [2] http://lkml.org/lkml/2012/8/18/75
> 
> 

-- 
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee

Re: Search for patch for kernel stack data disclosure in binfmt_script during execve

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

On Wed, Aug 22, 2012 at 09:49:35PM +0000, halfdog wrote:
> Got a hint via IRC, that I should not send patch idea for review to
> "generic" list, but to maintainers and last (or relevant) comitters of code.
> 
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=commitdiff;h=bf2a9a39639b8b51377905397a5005f444e9a892
> 
> CC to generic just for the records
> 
> halfdog wrote:
> > halfdog wrote:
> >> I'm searching for a patch for linux kernel stack disclosure in 
> >> binfmt_script with crafted interpreter names when CONFIG_MODULES
> >> is active (see [1]).
> > 
> > Please disregard my previous proposal [2], since it did not address
> > the problem directly (referencing local stack frame data from bprm
> > structure) but worked around it. I suspect, that this could increase
> > probability to reintroduce similar bugs.
> > 
> > Opinions on (untested sketch for) second solution: Could someone look
> > on the source code comments and changes in patch to judge, if this is
> > going in the right direction?
> > 
> > 
> > Explanation of patch: Since load_script will start to irreversibly
> > change bprm structures at some point (using stack local data was one
> > of those changes), try to delay this point. Run checks if load_script
> > could be the right handler, if not give other binfmt handlers the
> > chance to do so.
> > 
> > If binfmt_script is the right one, try to load the interpreter
> > (causing bprm modification), if failing make sure that no other binfmt
> > handler has the chance to continue on the now modified bprm data.
> > 
> > CAVEAT: This assumes, that if binfmt_script could handle the load,
> > that it would be the one and only binfmt with that ability, so no
> > other one, e.g. binfmt_misc should have the chance to do so. If this
> > assumption is wrong, leaving binfmt_script would have to rollback all
> > bprm changes (e.g. restore old credentials).
> > 
> > hd
> > 
> > [1]
> > http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
> > [2] http://lkml.org/lkml/2012/8/18/75

What about (untested):

diff --git a/fs/exec.c b/fs/exec.c
index 574cf4d..ef13850 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1438,7 +1438,8 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 		}
 		read_unlock(&binfmt_lock);
 #ifdef CONFIG_MODULES
-		if (retval != -ENOEXEC || bprm->mm == NULL) {
+		if (retval != -ENOEXEC || bprm->mm == NULL ||
+				bprm->recursion_depth > BINPRM_MAX_RECURSION) {
 			break;
 		} else {
 #define printable(c) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))
-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Search for patch for kernel stack data disclosure in binfmt_script during execve

[halfdog]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kirill A. Shutemov wrote:
> On Wed, Aug 22, 2012 at 09:49:35PM +0000, halfdog wrote:
>> Got a hint via IRC, that I should not send patch idea for review 
>> to "generic" list, but to maintainers and last (or relevant) 
>> comitters of code.
>> 
>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=commitdiff;h=bf2a9a39639b8b51377905397a5005f444e9a892
>>
>> 
...
>> halfdog wrote:
>>> halfdog wrote:
>>>> I'm searching for a patch for linux kernel stack disclosure 
>>>> in binfmt_script with crafted interpreter names when 
>>>> CONFIG_MODULES is active (see [1]).
>>> 
>>> Please disregard my previous proposal [2], since it did not 
>>> address the problem directly (referencing local stack frame 
>>> data from bprm structure) but worked around it. I suspect,
>>> that this could increase probability to reintroduce similar
>>> bugs.
>>> 
>>> Opinions on (untested sketch for) second solution: Could 
>>> someone look on the source code comments and changes in patch 
>>> to judge, if this is going in the right direction?
>>> 
>>> Explanation of patch: Since load_script will start to 
>>> irreversibly change bprm structures at some point (using stack 
>>> local data was one of those changes), try to delay this point. 
>>> Run checks if load_script could be the right handler, if not 
>>> give other binfmt handlers the chance to do so.
>>> 
>>> If binfmt_script is the right one, try to load the interpreter
>>>  (causing bprm modification), if failing make sure that no
>>> other binfmt handler has the chance to continue on the now
>>> modified bprm data.
>>> 
>>> CAVEAT: This assumes, that if binfmt_script could handle the 
>>> load, that it would be the one and only binfmt with that 
>>> ability, so no other one, e.g. binfmt_misc should have the 
>>> chance to do so. If this assumption is wrong, leaving 
>>> binfmt_script would have to rollback all bprm changes (e.g. 
>>> restore old credentials).
>>> 
>>> [1]
>>> http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
>>>
>>> 
[2] http://lkml.org/lkml/2012/8/18/75
> 
> What about (untested):
> 
> diff --git a/fs/exec.c b/fs/exec.c index 574cf4d..ef13850 100644 
> --- a/fs/exec.c +++ b/fs/exec.c @@ -1438,7 +1438,8 @@ int 
> search_binary_handler(struct linux_binprm *bprm,struct pt_regs 
> *regs) } read_unlock(&binfmt_lock); #ifdef CONFIG_MODULES -		if 
> (retval != -ENOEXEC || bprm->mm == NULL) { +		if (retval != 
> -ENOEXEC || bprm->mm == NULL || +				bprm->recursion_depth > 
> BINPRM_MAX_RECURSION) { break; } else { #define printable(c) 
> (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))

- - From my understanding, this patch should not fix the problem, since
recursion depth is reset back to old value after call of binfmt handler.
This is done, so that fs/exec does not have to trust all binfmts to
reset the depth by themselfes when leaving.

http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=fs/exec.c;h=da27b91ff1e8cbe87d0fe42aa5d39513e6a9deeb;hb=HEAD
   1408                         read_unlock(&binfmt_lock);
   1409                         retval = fn(bprm, regs);
   1410                         /*
   1411                          * Restore the depth counter to its
starting value
   1412                          * in this call, so we don't have to
rely on every
   1413                          * load_binary function to restore it on
return.
   1414                          */
   1415                         bprm->recursion_depth = depth;


I guess, the problem is, that recursion_depth usually not only limits
the depth, but also the maximal number of binfmt_xxx calls. That's why,
the use of local stack-frame data in bprm does not matter, there is no
going up the stack AND using bprm->interpreter, the last error is
terminates the search.

In the POC, search is not terminated because of ENOEXEC when max depth
reached and due to special filename, mod-loader triggers also (about 30
times? I do not known, if that could be a problem also, interfering with
other module loads. Usually non-root users cannot trigger rapid module
loads easily).

- -- 
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAlA3U3QACgkQxFmThv7tq+7hTgCZAcQFn70FUWnAhvoMYhm8EcFT
8vQAn06VtbeY5P0cPGd9fcxL6AaEF8oS
=An9g
-----END PGP SIGNATURE-----

[PATCH] Fix kernel stack data disclosure in binfmt_script during execve

[halfdog]

halfdog wrote:
> Kirill A. Shutemov wrote:
>> On Wed, Aug 22, 2012 at 09:49:35PM +0000, halfdog wrote:
>>> Got a hint via IRC, that I should not send patch idea for review 
>>> to "generic" list, but to maintainers and last (or relevant) 
>>> comitters of code.
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=commitdiff;h=bf2a9a39639b8b51377905397a5005f444e9a892
>>>
>>>
> ...
>>> halfdog wrote:
>>>> halfdog wrote:
>>>>> I'm searching for a patch for linux kernel stack disclosure 
>>>>> in binfmt_script with crafted interpreter names when 
>>>>> CONFIG_MODULES is active (see [1]).
>>>>
>>>> Please disregard my previous proposal [2], since it did not 
>>>> address the problem directly (referencing local stack frame 
>>>> data from bprm structure) but worked around it. I suspect,
>>>> that this could increase probability to reintroduce similar
>>>> bugs.
>>>>
>>>> Opinions on (untested sketch for) second solution: Could 
>>>> someone look on the source code comments and changes in patch 
>>>> to judge, if this is going in the right direction?
>>>>
>>>> Explanation of patch: Since load_script will start to 
>>>> irreversibly change bprm structures at some point (using stack 
>>>> local data was one of those changes), try to delay this point. 
>>>> Run checks if load_script could be the right handler, if not 
>>>> give other binfmt handlers the chance to do so.
>>>>
>>>> If binfmt_script is the right one, try to load the interpreter
>>>>  (causing bprm modification), if failing make sure that no
>>>> other binfmt handler has the chance to continue on the now
>>>> modified bprm data.
>>>>
>>>> CAVEAT: This assumes, that if binfmt_script could handle the 
>>>> load, that it would be the one and only binfmt with that 
>>>> ability, so no other one, e.g. binfmt_misc should have the 
>>>> chance to do so. If this assumption is wrong, leaving 
>>>> binfmt_script would have to rollback all bprm changes (e.g. 
>>>> restore old credentials).
>>>>
>>>> [1]
>>>> http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
>>>>
>>>>
> [2] http://lkml.org/lkml/2012/8/18/75
> 
>> What about (untested):
> 
>> diff --git a/fs/exec.c b/fs/exec.c index 574cf4d..ef13850 100644 
>> --- a/fs/exec.c +++ b/fs/exec.c @@ -1438,7 +1438,8 @@ int 
>> search_binary_handler(struct linux_binprm *bprm,struct pt_regs 
>> *regs) } read_unlock(&binfmt_lock); #ifdef CONFIG_MODULES -		if 
>> (retval != -ENOEXEC || bprm->mm == NULL) { +		if (retval != 
>> -ENOEXEC || bprm->mm == NULL || +				bprm->recursion_depth > 
>> BINPRM_MAX_RECURSION) { break; } else { #define printable(c) 
>> (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))
> 
> - From my understanding, this patch should not fix the problem, since
> recursion depth is reset back to old value after call of binfmt handler.
> This is done, so that fs/exec does not have to trust all binfmts to
> reset the depth by themselfes when leaving.
> 
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=fs/exec.c;h=da27b91ff1e8cbe87d0fe42aa5d39513e6a9deeb;hb=HEAD
>    1408                         read_unlock(&binfmt_lock);
>    1409                         retval = fn(bprm, regs);
>    1410                         /*
>    1411                          * Restore the depth counter to its
> starting value
>    1412                          * in this call, so we don't have to
> rely on every
>    1413                          * load_binary function to restore it on
> return.
>    1414                          */
>    1415                         bprm->recursion_depth = depth;
> 
> 
> I guess, the problem is, that recursion_depth usually not only limits
> the depth, but also the maximal number of binfmt_xxx calls. That's why,
> the use of local stack-frame data in bprm does not matter, there is no
> going up the stack AND using bprm->interpreter, the last error is
> terminates the search.
> 
> In the POC, search is not terminated because of ENOEXEC when max depth
> reached and due to special filename, mod-loader triggers also (about 30
> times? I do not known, if that could be a problem also, interfering with
> other module loads. Usually non-root users cannot trigger rapid module
> loads easily).

>> What about (untested):

Now this is the updated and also tested patch (vs. linux-3.5.4 kernel) to fix
https://bugzilla.kernel.org/show_bug.cgi?id=46841 . See also
http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
This patch adresses the stack data disclosure but does not deal with the
excessive recursion (to be handled in separate patch if needed).

--- fs/binfmt_script.c	2012-09-14 22:28:08.000000000 +0000
+++ fs/binfmt_script.c	2012-09-20 16:01:58.951942355 +0000
@@ -14,12 +14,24 @@
 #include <linux/err.h>
 #include <linux/fs.h>

+/** Check if this handler is suitable to load the "binary" identified
+ *  by first BINPRM_BUF_SIZE bytes in bprm->buf.
+ *  @returns -ENOEXEC if this handler is not suitable for that type
+ *  of binary. In that case, the handler must not modify any of the
+ *  data associated with bprm.
+ *  Any error if the binary should have been handled by this loader
+ *  but handling failed. In that case. FIXME: be defensive? also
+ *  kill bprm->mm or bprm->file also to make it impossible, that
+ *  upper search_binary_handler can continue handling?
+ *  0 (OK) otherwise, the new executable is ready in bprm->mm.
+ */
 static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
 {
 	const char *i_arg, *i_name;
 	char *cp;
 	struct file *file;
-	char interp[BINPRM_BUF_SIZE];
+	char bprm_buf_copy[BINPRM_BUF_SIZE];
+	const char *bprm_old_interp_name;
 	int retval;

 	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
@@ -30,25 +42,29 @@ static int load_script(struct linux_binp
 	 * Sorta complicated, but hopefully it will work.  -TYT
 	 */

-	bprm->recursion_depth++;
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+	/* Keep bprm unchanged until we known, that this is a script
+	 * to be handled by this loader. Copy bprm->buf for sure,
+	 * otherwise returning -ENOEXEC will make other handlers see
+	 * modified data. (hd)
+	 */
+	memcpy(bprm_buf_copy, bprm->buf, BINPRM_BUF_SIZE);

-	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-	if ((cp = strchr(bprm->buf, '\n')) == NULL)
-		cp = bprm->buf+BINPRM_BUF_SIZE-1;
+	bprm_buf_copy[BINPRM_BUF_SIZE - 1]='\0';
+	if ((cp = strchr(bprm_buf_copy, '\n')) == NULL)
+		cp = bprm_buf_copy+BINPRM_BUF_SIZE-1;
 	*cp = '\0';
-	while (cp > bprm->buf) {
+	while (cp > bprm_buf_copy) {
 		cp--;
 		if ((*cp == ' ') || (*cp == '\t'))
 			*cp = '\0';
 		else
 			break;
 	}
-	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
+	for (cp = bprm_buf_copy+2; (*cp == ' ') || (*cp == '\t'); cp++);
 	if (*cp == '\0')
-		return -ENOEXEC; /* No interpreter name found */
+	/* No interpreter name found. No problem to let other handlers
+	 * retry, we did not change anything. */
+		return -ENOEXEC;
 	i_name = cp;
 	i_arg = NULL;
 	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
@@ -57,45 +73,84 @@ static int load_script(struct linux_binp
 		*cp++ = '\0';
 	if (*cp)
 		i_arg = cp;
-	strcpy (interp, i_name);
+
+	/* So this is our point-of-no-return: modification of bprm
+	 * will be irreversible, so if we fail to setup execution
+	 * using the new interpreter name (i_name), we have to make
+	 * sure, that no other handler tries again. (hd)
+	 */
+
 	/*
 	 * OK, we've parsed out the interpreter name and
 	 * (optional) argument.
 	 * Splice in (1) the interpreter's name for argv[0]
-	 *           (2) (optional) argument to interpreter
-	 *           (3) filename of shell script (replace argv[0])
+	 *	   (2) (optional) argument to interpreter
+	 *	   (3) filename of shell script (replace argv[0])
 	 *
 	 * This is done in reverse order, because of how the
 	 * user environment and arguments are stored.
 	 */
+
+	/* Ugly: we store pointer to local stack frame in bprm,
+	 * so make sure to clear this up before returning.
+	 */
+	bprm_old_interp_name = bprm->interp;
+	bprm->interp = i_name;
+
 	retval = remove_arg_zero(bprm);
-	if (retval)
-		return retval;
-	retval = copy_strings_kernel(1, &bprm->interp, bprm);
-	if (retval < 0) return retval;
+	if (retval) goto out;
+	/* copy_strings_kernel is ok here, even when racy: since no
+	 * user can be attached to new mm, there is nobody to race
+	 * with and call is safe for now. The return code of
+	 * copy_strings_kernel cannot be -ENOEXEC in any case,
+	 * so no special checks needed. (hd)
+	 */
+	retval = copy_strings_kernel(1, &bprm_old_interp_name, bprm);
+	if (retval < 0) goto out;
 	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;
+	retval = copy_strings_kernel(1, &bprm->interp, bprm);
+	if (retval) goto out;
 	bprm->argc++;
-	bprm->interp = interp;

 	/*
 	 * OK, now restart the process with the interpreter's dentry.
+         * Release old file first
 	 */
-	file = open_exec(interp);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
-
+	allow_write_access(bprm->file);
+	fput(bprm->file);
+	bprm->file = NULL;
+	file = open_exec(bprm->interp);
+	if (IS_ERR(file)) {
+		retval=PTR_ERR(file);
+		goto out;
+        }
 	bprm->file = file;
+	/* Caveat: This also updates the credentials of the next exec. */
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		return retval;
-	return search_binary_handler(bprm,regs);
+		goto out;
+	bprm->recursion_depth++;
+	retval=search_binary_handler(bprm,regs);
+
+out:	/* Make sure, we do not return local stack frame data. If
+	 * it would be needed after returning, we would have needed
+	 * to allocate memory or use copy from new bprm->mm anyway. (hd)
+         */
+	bprm->interp = bprm_old_interp_name;
+	if(!retval) {
+		/* The handlers for starting of interpreter failed.
+		 * bprm is already modified, hence we are dead here.
+		 * Make sure, that we do not return -ENOEXEC, that would
+		 * allow searching for handlers to continue. (hd).
+		 */
+		if(retval==-ENOEXEC) retval=-EINVAL;
+	}
+	return(retval);
 }

 static struct linux_binfmt script_format = {

-- 
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee

Re: [PATCH] Fix kernel stack data disclosure in binfmt_script during execve

[Randy Dunlap]

On 09/20/2012 09:05 AM, halfdog wrote:

> halfdog wrote:
> 
> Now this is the updated and also tested patch (vs. linux-3.5.4 kernel) to fix
> https://bugzilla.kernel.org/show_bug.cgi?id=46841 . See also
> http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
> This patch adresses the stack data disclosure but does not deal with the
> excessive recursion (to be handled in separate patch if needed).
> 
> --- fs/binfmt_script.c	2012-09-14 22:28:08.000000000 +0000
> +++ fs/binfmt_script.c	2012-09-20 16:01:58.951942355 +0000


Incorrect diff/patch format for kernel patches.
It should be apply-able by using 'patch -p1'.

Oh, the patch is not signed off.
And Documentation/SubmittingPatches says signoffs are:

"using your real name (sorry, no pseudonyms or anonymous contributions.)"

There are also some kernel coding style issues that should be
fixed if this patch is to be merged.

> @@ -14,12 +14,24 @@
>  #include <linux/err.h>
>  #include <linux/fs.h>
> 
> +/** Check if this handler is suitable to load the "binary" identified


/** means kernel-doc notation in the kernel, but this comment
block is not kernel-doc, so don't start it with /**

> + *  by first BINPRM_BUF_SIZE bytes in bprm->buf.
> + *  @returns -ENOEXEC if this handler is not suitable for that type


We don't use "@returns", just returns: <text>.

> + *  of binary. In that case, the handler must not modify any of the
> + *  data associated with bprm.
> + *  Any error if the binary should have been handled by this loader
> + *  but handling failed. In that case. FIXME: be defensive? also
> + *  kill bprm->mm or bprm->file also to make it impossible, that
> + *  upper search_binary_handler can continue handling?
> + *  0 (OK) otherwise, the new executable is ready in bprm->mm.
> + */
>  static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
>  {
>  	const char *i_arg, *i_name;
>  	char *cp;
>  	struct file *file;
> -	char interp[BINPRM_BUF_SIZE];
> +	char bprm_buf_copy[BINPRM_BUF_SIZE];
> +	const char *bprm_old_interp_name;
>  	int retval;
> 
>  	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
> @@ -30,25 +42,29 @@ static int load_script(struct linux_binp
>  	 * Sorta complicated, but hopefully it will work.  -TYT
>  	 */
> 
> -	bprm->recursion_depth++;
> -	allow_write_access(bprm->file);
> -	fput(bprm->file);
> -	bprm->file = NULL;
> +	/* Keep bprm unchanged until we known, that this is a script
> +	 * to be handled by this loader. Copy bprm->buf for sure,
> +	 * otherwise returning -ENOEXEC will make other handlers see
> +	 * modified data. (hd)
> +	 */


Kernel multi-line comment style is
	/*
	 * line 1
	 * line 2
	 */

> +	memcpy(bprm_buf_copy, bprm->buf, BINPRM_BUF_SIZE);
> 
> -	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
> -	if ((cp = strchr(bprm->buf, '\n')) == NULL)
> -		cp = bprm->buf+BINPRM_BUF_SIZE-1;
> +	bprm_buf_copy[BINPRM_BUF_SIZE - 1]='\0';
> +	if ((cp = strchr(bprm_buf_copy, '\n')) == NULL)
> +		cp = bprm_buf_copy+BINPRM_BUF_SIZE-1;
>  	*cp = '\0';
> -	while (cp > bprm->buf) {
> +	while (cp > bprm_buf_copy) {
>  		cp--;
>  		if ((*cp == ' ') || (*cp == '\t'))
>  			*cp = '\0';
>  		else
>  			break;
>  	}
> -	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
> +	for (cp = bprm_buf_copy+2; (*cp == ' ') || (*cp == '\t'); cp++);
>  	if (*cp == '\0')
> -		return -ENOEXEC; /* No interpreter name found */
> +	/* No interpreter name found. No problem to let other handlers
> +	 * retry, we did not change anything. */


multi-line comment style.

> +		return -ENOEXEC;
>  	i_name = cp;
>  	i_arg = NULL;
>  	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
> @@ -57,45 +73,84 @@ static int load_script(struct linux_binp
>  		*cp++ = '\0';
>  	if (*cp)
>  		i_arg = cp;
> -	strcpy (interp, i_name);
> +
> +	/* So this is our point-of-no-return: modification of bprm
> +	 * will be irreversible, so if we fail to setup execution
> +	 * using the new interpreter name (i_name), we have to make
> +	 * sure, that no other handler tries again. (hd)
> +	 */


ditto.

> +
>  	/*
>  	 * OK, we've parsed out the interpreter name and
>  	 * (optional) argument.
>  	 * Splice in (1) the interpreter's name for argv[0]
> -	 *           (2) (optional) argument to interpreter
> -	 *           (3) filename of shell script (replace argv[0])
> +	 *	   (2) (optional) argument to interpreter
> +	 *	   (3) filename of shell script (replace argv[0])
>  	 *
>  	 * This is done in reverse order, because of how the
>  	 * user environment and arguments are stored.
>  	 */
> +
> +	/* Ugly: we store pointer to local stack frame in bprm,
> +	 * so make sure to clear this up before returning.
> +	 */


ditto.

> +	bprm_old_interp_name = bprm->interp;
> +	bprm->interp = i_name;
> +
>  	retval = remove_arg_zero(bprm);
> -	if (retval)
> -		return retval;
> -	retval = copy_strings_kernel(1, &bprm->interp, bprm);
> -	if (retval < 0) return retval;
> +	if (retval) goto out;


Really should be
	if (retval)
		goto out;

> +	/* copy_strings_kernel is ok here, even when racy: since no
> +	 * user can be attached to new mm, there is nobody to race
> +	 * with and call is safe for now. The return code of
> +	 * copy_strings_kernel cannot be -ENOEXEC in any case,
> +	 * so no special checks needed. (hd)
> +	 */


style.

> +	retval = copy_strings_kernel(1, &bprm_old_interp_name, bprm);
> +	if (retval < 0) goto out;
>  	bprm->argc++;
>  	if (i_arg) {
>  		retval = copy_strings_kernel(1, &i_arg, bprm);
> -		if (retval < 0) return retval;
> +		if (retval < 0) goto out;


style.

>  		bprm->argc++;
>  	}
> -	retval = copy_strings_kernel(1, &i_name, bprm);
> -	if (retval) return retval;
> +	retval = copy_strings_kernel(1, &bprm->interp, bprm);
> +	if (retval) goto out;


style.  (but Al can override these if he wants to)

>  	bprm->argc++;
> -	bprm->interp = interp;
> 
>  	/*
>  	 * OK, now restart the process with the interpreter's dentry.
> +         * Release old file first


indentation mucked up.

>  	 */
> -	file = open_exec(interp);
> -	if (IS_ERR(file))
> -		return PTR_ERR(file);
> -
> +	allow_write_access(bprm->file);
> +	fput(bprm->file);
> +	bprm->file = NULL;
> +	file = open_exec(bprm->interp);
> +	if (IS_ERR(file)) {
> +		retval=PTR_ERR(file);
> +		goto out;
> +        }
>  	bprm->file = file;
> +	/* Caveat: This also updates the credentials of the next exec. */
>  	retval = prepare_binprm(bprm);
>  	if (retval < 0)
> -		return retval;
> -	return search_binary_handler(bprm,regs);
> +		goto out;
> +	bprm->recursion_depth++;
> +	retval=search_binary_handler(bprm,regs);
> +
> +out:	/* Make sure, we do not return local stack frame data. If
> +	 * it would be needed after returning, we would have needed
> +	 * to allocate memory or use copy from new bprm->mm anyway. (hd)
> +         */


Comment block probably should come before the label.
and the indentation is mucked up.

> +	bprm->interp = bprm_old_interp_name;
> +	if(!retval) {
> +		/* The handlers for starting of interpreter failed.
> +		 * bprm is already modified, hence we are dead here.
> +		 * Make sure, that we do not return -ENOEXEC, that would
> +		 * allow searching for handlers to continue. (hd).
> +		 */

style

> +		if(retval==-ENOEXEC) retval=-EINVAL;


missing space before '('.
etc.

> +	}
> +	return(retval);
>  }
> 
>  static struct linux_binfmt script_format = {
> 



-- 
~Randy

Re: [PATCH v2] Fix kernel stack data disclosure in binfmt_script during execve

[halfdog]

Randy Dunlap wrote:
> On 09/20/2012 09:05 AM, halfdog wrote:
> 
>> halfdog wrote:
>>
>> Now this is the updated and also tested patch (vs. linux-3.5.4 kernel) to fix
>> https://bugzilla.kernel.org/show_bug.cgi?id=46841 . See also
>> http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
>> This patch adresses the stack data disclosure but does not deal with the
>> excessive recursion (to be handled in separate patch if needed).
>>
>> --- fs/binfmt_script.c	2012-09-14 22:28:08.000000000 +0000
>> +++ fs/binfmt_script.c	2012-09-20 16:01:58.951942355 +0000
> 
> 
> Incorrect diff/patch format for kernel patches.
> It should be apply-able by using 'patch -p1'.
> ...

OK, formatting changed:
* patch depth level added
* comment style changed
* goto-s now on own line

Has any one looked at the logic apart from the styling? Are there any flaws?

> Oh, the patch is not signed off.

Yes. Anyone who likes it can sign it off or even resubmit it in his name.

--- linux-3.5.4/fs/binfmt_script.c	2012-09-14 22:28:08.000000000 +0000
+++ linux-3.5.4/fs/binfmt_script.c	2012-09-23 02:28:39.905123091 +0000
@@ -14,12 +14,25 @@
 #include <linux/err.h>
 #include <linux/fs.h>

+/*
+ *  Check if this handler is suitable to load the "binary" identified
+ *  by first BINPRM_BUF_SIZE bytes in bprm->buf.
+ *  returns: -ENOEXEC if this handler is not suitable for that type
+ *  of binary. In that case, the handler must not modify any of the
+ *  data associated with bprm.
+ *  Any error if the binary should have been handled by this loader
+ *  but handling failed. In that case. FIXME: be defensive? also
+ *  kill bprm->mm or bprm->file also to make it impossible, that
+ *  upper search_binary_handler can continue handling?
+ *  0 (OK) otherwise, the new executable is ready in bprm->mm.
+ */
 static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
 {
 	const char *i_arg, *i_name;
 	char *cp;
 	struct file *file;
-	char interp[BINPRM_BUF_SIZE];
+	char bprm_buf_copy[BINPRM_BUF_SIZE];
+	const char *bprm_old_interp_name;
 	int retval;

 	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
@@ -30,25 +43,32 @@ static int load_script(struct linux_binp
 	 * Sorta complicated, but hopefully it will work.  -TYT
 	 */

-	bprm->recursion_depth++;
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+	/*
+	 * Keep bprm unchanged until we known, that this is a script
+	 * to be handled by this loader. Copy bprm->buf for sure,
+	 * otherwise returning -ENOEXEC will make other handlers see
+	 * modified data. (hd)
+	 */
+	memcpy(bprm_buf_copy, bprm->buf, BINPRM_BUF_SIZE);

-	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-	if ((cp = strchr(bprm->buf, '\n')) == NULL)
-		cp = bprm->buf+BINPRM_BUF_SIZE-1;
+	bprm_buf_copy[BINPRM_BUF_SIZE - 1]='\0';
+	if ((cp = strchr(bprm_buf_copy, '\n')) == NULL)
+		cp = bprm_buf_copy+BINPRM_BUF_SIZE-1;
 	*cp = '\0';
-	while (cp > bprm->buf) {
+	while (cp > bprm_buf_copy) {
 		cp--;
 		if ((*cp == ' ') || (*cp == '\t'))
 			*cp = '\0';
 		else
 			break;
 	}
-	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
+	for (cp = bprm_buf_copy+2; (*cp == ' ') || (*cp == '\t'); cp++);
 	if (*cp == '\0')
-		return -ENOEXEC; /* No interpreter name found */
+	/*
+	 * No interpreter name found. No problem to let other handlers
+	 * retry, we did not change anything.
+	 */
+		return -ENOEXEC;
 	i_name = cp;
 	i_arg = NULL;
 	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
@@ -57,45 +77,94 @@ static int load_script(struct linux_binp
 		*cp++ = '\0';
 	if (*cp)
 		i_arg = cp;
-	strcpy (interp, i_name);
+
+	/*
+	 * So this is our point-of-no-return: modification of bprm
+	 * will be irreversible, so if we fail to setup execution
+	 * using the new interpreter name (i_name), we have to make
+	 * sure, that no other handler tries again. (hd)
+	 */
+
 	/*
 	 * OK, we've parsed out the interpreter name and
 	 * (optional) argument.
 	 * Splice in (1) the interpreter's name for argv[0]
-	 *           (2) (optional) argument to interpreter
-	 *           (3) filename of shell script (replace argv[0])
+	 *	   (2) (optional) argument to interpreter
+	 *	   (3) filename of shell script (replace argv[0])
 	 *
 	 * This is done in reverse order, because of how the
 	 * user environment and arguments are stored.
 	 */
+
+	/*
+	 * Ugly: we store pointer to local stack frame in bprm,
+	 * so make sure to clear this up before returning.
+	 */
+	bprm_old_interp_name = bprm->interp;
+	bprm->interp = i_name;
+
 	retval = remove_arg_zero(bprm);
 	if (retval)
-		return retval;
-	retval = copy_strings_kernel(1, &bprm->interp, bprm);
-	if (retval < 0) return retval;
+		goto out;
+	/*
+	 * copy_strings_kernel is ok here, even when racy: since no
+	 * user can be attached to new mm, there is nobody to race
+	 * with and call is safe for now. The return code of
+	 * copy_strings_kernel cannot be -ENOEXEC in any case,
+	 * so no special checks needed. (hd)
+	 */
+	retval = copy_strings_kernel(1, &bprm_old_interp_name, bprm);
+	if (retval < 0)
+		goto out;
 	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;
+	retval = copy_strings_kernel(1, &bprm->interp, bprm);
+	if (retval)
+		goto out;
 	bprm->argc++;
-	bprm->interp = interp;

 	/*
 	 * OK, now restart the process with the interpreter's dentry.
+	 * Release old file first
 	 */
-	file = open_exec(interp);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
-
+	allow_write_access(bprm->file);
+	fput(bprm->file);
+	bprm->file = NULL;
+	file = open_exec(bprm->interp);
+	if (IS_ERR(file)) {
+		retval=PTR_ERR(file);
+		goto out;
+	}
 	bprm->file = file;
+	/* Caveat: This also updates the credentials of the next exec. */
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
-		return retval;
-	return search_binary_handler(bprm,regs);
+		goto out;
+	bprm->recursion_depth++;
+	retval=search_binary_handler(bprm,regs);
+
+	/*
+	 * Make sure, we do not return local stack frame data. If
+	 * it would be needed after returning, we would have needed
+	 * to allocate memory or use copy from new bprm->mm anyway. (hd)
+	 */
+out:
+	bprm->interp = bprm_old_interp_name;
+	if(!retval) {
+		/*
+		 * The handlers for starting of interpreter failed.
+		 * bprm is already modified, hence we are dead here.
+		 * Make sure, that we do not return -ENOEXEC, that would
+		 * allow searching for handlers to continue. (hd).
+		 */
+		if(retval==-ENOEXEC) retval=-EINVAL;
+	}
+	return(retval);
 }

 static struct linux_binfmt script_format = {
---
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee