Re: Bug#772807: binfmt-support: unable to close /proc/sys/fs/binfmt_misc/register: Invalid argument
From: Arthur Marsh
Date: Sun Dec 14 2014 - 03:34:13 EST
Al Viro wrote on 12/12/14 16:31:
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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>
---
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);
}
I had to revert Al Viro's "Undo some of the "prettifying" braindamage."
patch above to apply the execveat() commit below that is now in Linus'
git head:
https://github.com/torvalds/linux/commit/51f39a1f0cea1cacf8c787f652f26dfee9611874
After that, trying to apply Al Viro's patch resulted in on failed hunk
and code that wouldn't build.
binfmt_misc.c building but not working doesn't break things for me, but
it would be good to get fixed.
Regards,
Arthur.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/