Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756253AbaLLGBu (ORCPT ); Fri, 12 Dec 2014 01:01:50 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:50431 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753789AbaLLGBt (ORCPT ); Fri, 12 Dec 2014 01:01:49 -0500 Date: Fri, 12 Dec 2014 06:01:38 +0000 From: Al Viro To: Arthur Marsh Cc: Colin Watson , 772807@bugs.debian.org, linux-kernel@vger.kernel.org, vapier@gentoo.org, Joe Perches Subject: Re: Bug#772807: binfmt-support: unable to close /proc/sys/fs/binfmt_misc/register: Invalid argument Message-ID: <20141212060138.GC22149@ZenIV.linux.org.uk> References: <20141211102439.5047.90481.reportbug@localhost> <20141211104408.GQ3020@riva.ucam.org> <548987B8.8090202@internode.on.net> <20141211124024.GR3020@riva.ucam.org> <548A6D63.3010106@internode.on.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <548A6D63.3010106@internode.on.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 12, 2014 at 02:51:55PM +1030, Arthur Marsh wrote: > 6b899c4e9a049dfca759d990bd53b14f81c3626c is the first bad commit > commit 6b899c4e9a049dfca759d990bd53b14f81c3626c > Author: Mike Frysinger > 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 --- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/