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 <vap...@gentoo.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
signature.asc
Description: Digital signature
