Re: Bug#772807: binfmt-support: unable to close /proc/sys/fs/binfmt_misc/register: Invalid argument

From: Mike Frysinger
Date: Wed Dec 31 2014 - 01:23:22 EST


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

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

Attachment: signature.asc
Description: Digital signature