Search for patch for kernel stack disclosure in binfmt_script during execve

@@ -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 = {

index 574cf4d..ef13850 100644
@@ -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))

@@ -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 = {

@@ -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 = {