binfmt_misc broken

binfmt_misc broken

[Jeff Chua]

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 {

Re: binfmt_misc broken

[Linus Torvalds]

On Mon, Jun 10, 2013 at 6:52 AM, Jeff Chua <jeff.chua.linux@gmail.com> 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

Re: binfmt_misc broken

[Al Viro]

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...

Re: binfmt_misc broken

[Jeff Chua]

On Tue, Jun 11, 2013 at 9:51 AM, Al Viro <viro@zeniv.linux.org.uk> 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