2013-06-10 13:52:52

by Jeff Chua

[permalink] [raw]
Subject: binfmt_misc broken



According to Documentation/binfmt_misc.txt, the 'magic' and 'mask' can be
set by echoing it to /proc/sys/fs/binfmt_misc/register.

Here's the problem I can across while working on ARM.

# echo
':arm:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x28\x00:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/bin/qemu-arm-static:'
>/proc/sys/fs/binfmt_misc/register

# cat /proc/sys/fs/binfmt_misc/arm
wrong ...
magic 7f454c46010101
mask ffffffffffffff

right ...
magic 7f454c4601010100000000000000000002002800
mask ffffffffffffff00fffffffffffffffffeffffff


binfmt_misc is truncating e->size, so once ARM's magic is loaded, 32-bit
x86 can no longer run.

Here's a patch for it. It's looking for the delimiter ":" instead of \0.
Now 32-bit x86 can run concurrent while qemu-arm is handling ARM's magic.

Thanks,
Jeff


--- linux/fs/binfmt_misc.c 2013-05-01 12:59:39.000000000 +0800
+++ linux/fs/binfmt_misc.c 2013-06-10 21:29:45.000000000 +0800
@@ -276,6 +276,7 @@
int memsize, err;
char *buf, *p;
char del;
+ int esize = 0;

/* some sanity checks */
err = -EINVAL;
@@ -323,6 +324,15 @@
e->offset = simple_strtoul(p, &p, 10);
if (*p++)
goto Einval;
+
+ while(*(p + esize) != ':' && esize < BINPRM_BUF_SIZE) {
+ if(*(p + esize) == '\\' && *(p + esize + 1) == 'x') {
+ esize +=3;
+ } else
+ esize++;
+ }
+ e->size = esize;
+
e->magic = p;
p = scanarg(p, del);
if (!p)
@@ -337,10 +347,6 @@
p[-1] = '\0';
if (!e->mask[0])
e->mask = NULL;
- e->size = string_unescape_inplace(e->magic, UNESCAPE_HEX);
- if (e->mask &&
- string_unescape_inplace(e->mask, UNESCAPE_HEX) != e->size)
- goto Einval;
if (e->size + e->offset > BINPRM_BUF_SIZE)
goto Einval;
} else {


2013-06-10 20:04:16

by Linus Torvalds

[permalink] [raw]
Subject: Re: binfmt_misc broken

On Mon, Jun 10, 2013 at 6:52 AM, Jeff Chua <[email protected]> wrote:
>
> binfmt_misc is truncating e->size, so once ARM's magic is loaded, 32-bit x86
> can no longer run.

That patch is really ugly. And it doesn't make much sense. Where does
it now turn the hex into binary? And where does it check that the mask
is the same size as the data? You have changed the meaning of "esize"
to be the size of the original string, which is just wrong, and makes
no sense, since it has to be the same value for magic and for mask. So
the patch seems to make things just worse.

That said, there does seem to be *real* bugs, like "check_file()", that does

if (p && !strcmp(e->magic, p + 1))

which seems wrong. I think it should use "memcmp( ..., e->size)" instead.

And from your /proc output, esize does get truncated. But where
exactly does that happen? Is string_unescape() just broken? The code
*shouldn't* look at zeroes in the magic/mask strings, because they
should all be treated as memory regions with size 'e->size'.

Linus

2013-06-11 01:51:52

by Al Viro

[permalink] [raw]
Subject: Re: binfmt_misc broken

On Mon, Jun 10, 2013 at 09:52:44PM +0800, Jeff Chua wrote:
>
>
> According to Documentation/binfmt_misc.txt, the 'magic' and 'mask'
> can be set by echoing it to /proc/sys/fs/binfmt_misc/register.
>
> Here's the problem I can across while working on ARM.
>
> # echo ':arm:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x28\x00:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/bin/qemu-arm-static:'
> >/proc/sys/fs/binfmt_misc/register
>
> # cat /proc/sys/fs/binfmt_misc/arm
> wrong ...
> magic 7f454c46010101
> mask ffffffffffffff
>
> right ...
> magic 7f454c4601010100000000000000000002002800
> mask ffffffffffffff00fffffffffffffffffeffffff
>
>
> binfmt_misc is truncating e->size, so once ARM's magic is loaded,
> 32-bit x86 can no longer run.
>
> Here's a patch for it. It's looking for the delimiter ":" instead of
> \0. Now 32-bit x86 can run concurrent while qemu-arm is handling
> ARM's magic.

Patch is complete BS and I really wonder what kernel have you observed that bug on -
with mainline on amd64 your example yields
root@kvm-amd64:~# cat /proc/sys/fs/binfmt_misc/arm
enabled
interpreter /usr/bin/qemu-arm-static
flags:
offset 0
magic 7f454c4601010100000000000000000002002800
mask ffffffffffffff00fffffffffffffffffeffffff

A reproducer, please... As for the memcmp() Linus has suggested - it's !Magic case, i.e.
what we are comparing there is not the file contents, it's the extension. IOW, strcmp()
is the right thing to use there - pathnames do not contain NULs in the middle...

2013-06-11 02:17:18

by Jeff Chua

[permalink] [raw]
Subject: Re: binfmt_misc broken

On Tue, Jun 11, 2013 at 9:51 AM, Al Viro <[email protected]> wrote:


> Patch is complete BS and I really wonder what kernel have you observed that bug on -
> with mainline on amd64 your example yields
> root@kvm-amd64:~# cat /proc/sys/fs/binfmt_misc/arm
> enabled
> interpreter /usr/bin/qemu-arm-static
> flags:
> offset 0
> magic 7f454c4601010100000000000000000002002800
> mask ffffffffffffff00fffffffffffffffffeffffff
>
> A reproducer, please... As for the memcmp() Linus has suggested - it's !Magic case, i.e.
> what we are comparing there is not the file contents, it's the extension. IOW, strcmp()
> is the right thing to use there - pathnames do not contain NULs in the middle...

BS ... yes, after testing it again, you may be right. Not intented, sorry.

I did another test with bash.

# bash -version
GNU bash, version 4.2.45(2)-release (x86_64-unknown-linux-gnu)

# echo ':arm:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x28\x00:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/bin/qemu-arm-static:'
:arm:M::ELF(:???????????????????:/usr/bin/qemu-arm-static:

# echo ':arm:M::\\x7fELF\\x01\\x01\\x01\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x02\\x00\\x28\\x00:\\xff\\xff\\xff\\xff\\xff\\xff\\xff\\x00\\xff\\xff\\xff\\xff\\xff\\xff\\xff\\xff\\xfe\\xff\\xff\\xff:/usr/bin/qemu-arm-static:'
:arm:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x28\x00:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/bin/qemu-arm-static:

I supposed it's my bash configured with opt_xpg_echo=yes that's
sending in different data to the kernel.

Sending in the double-escape solved the problem. BS totally! My fault.

Thanks,
Jeff