Message-ID: <20141231062303.GE3906@vapier.wh0rd.info>
Date:	Wed, 31 Dec 2014 01:23:03 -0500
From:	Mike Frysinger <vapier@...too.org>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Arthur Marsh <arthur.marsh@...ernode.on.net>,
	Colin Watson <cjwatson@...ian.org>, 772807@...s.debian.org,
	linux-kernel@...r.kernel.org, Joe Perches <joe@...ches.com>
Subject: Re: Bug#772807: binfmt-support: unable to close
 /proc/sys/fs/binfmt_misc/register: Invalid argument

On 12 Dec 2014 06:01, Al Viro wrote:
> 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.

the checks are not correct.  magic & mask are binary fields hence checking for a 
NUL byte to indicate string parsing failed makes no sense.  your patch restores 
the bug i attempted to fix -- if i wanted to ignore the first byte of the file 
by setting the mask or magic to 0, then binfmt_misc will wrongly reject it.

the current code does reject some bad inputs -- namely, when the magic or mask 
is not specified.  i was counting on the scanarg not incrementing the pointer in 
that case, but as you pointed out, that doesn't work since the func always 
increments the pointer.  i'll see if i can handle both cases.

> Subject: unfuck fs/binfmt_misc.c
> 
> Undo some of the "prettifying" braindamage.

commit 7d65cf10e3d7747033b83fa18c5f3d2a498f66bc has merged at this point, but 
the attribution to e6084d4 is wrong.  of course coding style changes & 
functional changes shouldn't be done which is why i didn't do it.  the change 
you're referring to above has nothing to do e6084d4 as it was added before that 
in 6b899c4e9a049dfca759d990bd53b14f81c3626c (where i added extended debug 
output).  arguably those few non-debug related lines shouldn't be in that 
commit, but it's a long cry from style changes.
-mike

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists