lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 12 Dec 2014 06:01:38 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Arthur Marsh <arthur.marsh@...ernode.on.net>
Cc:	Colin Watson <cjwatson@...ian.org>, 772807@...s.debian.org,
	linux-kernel@...r.kernel.org, vapier@...too.org,
	Joe Perches <joe@...ches.com>
Subject: Re: Bug#772807: binfmt-support: unable to close
 /proc/sys/fs/binfmt_misc/register: Invalid argument

On Fri, Dec 12, 2014 at 02:51:55PM +1030, Arthur Marsh wrote:
> 6b899c4e9a049dfca759d990bd53b14f81c3626c is the first bad commit
> commit 6b899c4e9a049dfca759d990bd53b14f81c3626c
> Author: Mike Frysinger <vapier@...too.org>
> Date:   Wed Dec 10 15:52:08 2014 -0800
> 
>     binfmt_misc: add comments & debug logs
> 
>     When trying to develop a custom format handler, the errors returned all
>     effectively get bucketed as EINVAL with no kernel messages.  The other
>     errors (ENOMEM/EFAULT) are internal/obvious and basic.  Thus any time a
>     bad handler is rejected, the developer has to walk the dense code and
>     try to guess where it went wrong.  Needing to dive into kernel code is
>     itself a fairly high barrier for a lot of people.
> 
>     To improve this situation, let's deploy extensive pr_debug markers at
>     logical parse points, and add comments to the dense parsing logic.  It
>     let's you see exactly where the parsing aborts, the string the kernel
>     received (useful when dealing with shell code), how it translated the
>     buffers to binary data, and how it will apply the mask at runtime.

... and looking at it shows the following garbage:
                p[-1] = '\0';
-               if (!e->magic[0])
+               if (p == e->magic)

That code makes no sense whatsoever - if p *was* equal to e->magic, the
assignment before it would be obviously broken.  And a few lines later we
have another chunk just like that.

Moreover, if you look at further context, you'll see that it's
                e->magic = p;
                p = scanarg(p, del);
                if (!p)
                        sod off
                p[-1] = '\0';
                if (p == e->magic)
			sod off
Now, that condition could be true only if scanarg(p, del) would return p,
right?  Let's take a look at scanarg():
static char *scanarg(char *s, char del)
{
        char c;

        while ((c = *s++) != del) {
                if (c == '\\' && *s == 'x') {
                        s++;  
                        if (!isxdigit(*s++)) 
                                return NULL;
                        if (!isxdigit(*s++))
                                return NULL;
                }
        }
        return s;
}

See that s++ in the loop condition?  There's no way in hell it would *not*
increment s.  If we return non-NULL, we know that c was equal to del *and*
c is equal to previous_value_of_s[0], i.e. s[-1].  IOW, return value points
to the byte following the first instance of delimeter starting at the argument.

And p[-1] = '\0' replaces delimiter with NUL.  IOW, the checks used to be
correct.  And got buggered.

Subject: unfuck fs/binfmt_misc.c

Undo some of the "prettifying" braindamage.

Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
---
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 70789e1..a6b6697 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -5,6 +5,8 @@
  *
  * binfmt_misc detects binaries via a magic or filename extension and invokes
  * a specified wrapper. See Documentation/binfmt_misc.txt for more details.
+ *
+ * Cetero censeo, checkpatch.pl esse delendam
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -131,9 +133,8 @@ static int load_misc_binary(struct linux_binprm *bprm)
 	int retval;
 	int fd_binary = -1;
 
-	retval = -ENOEXEC;
 	if (!enabled)
-		goto ret;
+		return -ENOEXEC;
 
 	/* to keep locking time low, we copy the interpreter string */
 	read_lock(&entries_lock);
@@ -142,12 +143,12 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		strlcpy(iname, fmt->interpreter, BINPRM_BUF_SIZE);
 	read_unlock(&entries_lock);
 	if (!fmt)
-		goto ret;
+		return -ENOEXEC;
 
 	if (!(fmt->flags & MISC_FMT_PRESERVE_ARGV0)) {
 		retval = remove_arg_zero(bprm);
 		if (retval)
-			goto ret;
+			return retval;
 	}
 
 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
@@ -157,10 +158,9 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		 * to it
 		 */
 		fd_binary = get_unused_fd_flags(0);
-		if (fd_binary < 0) {
-			retval = fd_binary;
-			goto ret;
-		}
+		if (fd_binary < 0)
+			return fd_binary;
+
 		fd_install(fd_binary, bprm->file);
 
 		/* if the binary is not readable than enforce mm->dumpable=0
@@ -219,14 +219,13 @@ static int load_misc_binary(struct linux_binprm *bprm)
 	if (retval < 0)
 		goto error;
 
-ret:
 	return retval;
 error:
 	if (fd_binary > 0)
 		sys_close(fd_binary);
 	bprm->interp_flags = 0;
 	bprm->interp_data = 0;
-	goto ret;
+	return retval;
 }
 
 /* Command parsers */
@@ -250,6 +249,7 @@ static char *scanarg(char *s, char del)
 				return NULL;
 		}
 	}
+	s[-1] = '\0';
 	return s;
 }
 
@@ -316,7 +316,7 @@ static Node *create_entry(const char __user *buffer, size_t count)
 
 	memset(e, 0, sizeof(Node));
 	if (copy_from_user(buf, buffer, count))
-		goto efault;
+		goto Efault;
 
 	del = *p++;	/* delimeter */
 
@@ -329,13 +329,13 @@ static Node *create_entry(const char __user *buffer, size_t count)
 	e->name = p;
 	p = strchr(p, del);
 	if (!p)
-		goto einval;
+		goto Einval;
 	*p++ = '\0';
 	if (!e->name[0] ||
 	    !strcmp(e->name, ".") ||
 	    !strcmp(e->name, "..") ||
 	    strchr(e->name, '/'))
-		goto einval;
+		goto Einval;
 
 	pr_debug("register: name: {%s}\n", e->name);
 
@@ -350,10 +350,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		e->flags = (1 << Enabled) | (1 << Magic);
 		break;
 	default:
-		goto einval;
+		goto Einval;
 	}
 	if (*p++ != del)
-		goto einval;
+		goto Einval;
 
 	if (test_bit(Magic, &e->flags)) {
 		/* Handle the 'M' (magic) format. */
@@ -362,21 +362,20 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		/* Parse the 'offset' field. */
 		s = strchr(p, del);
 		if (!s)
-			goto einval;
+			goto Einval;
 		*s++ = '\0';
 		e->offset = simple_strtoul(p, &p, 10);
 		if (*p++)
-			goto einval;
+			goto Einval;
 		pr_debug("register: offset: %#x\n", e->offset);
 
 		/* Parse the 'magic' field. */
 		e->magic = p;
 		p = scanarg(p, del);
 		if (!p)
-			goto einval;
-		p[-1] = '\0';
-		if (p == e->magic)
-			goto einval;
+			goto Einval;
+		if (!e->magic[0])
+			goto Einval;
 		if (USE_DEBUG)
 			print_hex_dump_bytes(
 				KBUILD_MODNAME ": register: magic[raw]: ",
@@ -386,9 +385,8 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		e->mask = p;
 		p = scanarg(p, del);
 		if (!p)
-			goto einval;
-		p[-1] = '\0';
-		if (p == e->mask) {
+			goto Einval;
+		if (!e->mask[0]) {
 			e->mask = NULL;
 			pr_debug("register:  mask[raw]: none\n");
 		} else if (USE_DEBUG)
@@ -405,9 +403,9 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		e->size = string_unescape_inplace(e->magic, UNESCAPE_HEX);
 		if (e->mask &&
 		    string_unescape_inplace(e->mask, UNESCAPE_HEX) != e->size)
-			goto einval;
+			goto Einval;
 		if (e->size + e->offset > BINPRM_BUF_SIZE)
-			goto einval;
+			goto Einval;
 		pr_debug("register: magic/mask length: %i\n", e->size);
 		if (USE_DEBUG) {
 			print_hex_dump_bytes(
@@ -439,23 +437,23 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		/* Skip the 'offset' field. */
 		p = strchr(p, del);
 		if (!p)
-			goto einval;
+			goto Einval;
 		*p++ = '\0';
 
 		/* Parse the 'magic' field. */
 		e->magic = p;
 		p = strchr(p, del);
 		if (!p)
-			goto einval;
+			goto Einval;
 		*p++ = '\0';
 		if (!e->magic[0] || strchr(e->magic, '/'))
-			goto einval;
+			goto Einval;
 		pr_debug("register: extension: {%s}\n", e->magic);
 
 		/* Skip the 'mask' field. */
 		p = strchr(p, del);
 		if (!p)
-			goto einval;
+			goto Einval;
 		*p++ = '\0';
 	}
 
@@ -463,10 +461,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
 	e->interpreter = p;
 	p = strchr(p, del);
 	if (!p)
-		goto einval;
+		goto Einval;
 	*p++ = '\0';
 	if (!e->interpreter[0])
-		goto einval;
+		goto Einval;
 	pr_debug("register: interpreter: {%s}\n", e->interpreter);
 
 	/* Parse the 'flags' field. */
@@ -474,17 +472,17 @@ static Node *create_entry(const char __user *buffer, size_t count)
 	if (*p == '\n')
 		p++;
 	if (p != buf + count)
-		goto einval;
+		goto Einval;
 
 	return e;
 
 out:
 	return ERR_PTR(err);
 
-efault:
+Efault:
 	kfree(e);
 	return ERR_PTR(-EFAULT);
-einval:
+Einval:
 	kfree(e);
 	return ERR_PTR(-EINVAL);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists