linux-kernel - Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 Nov 2015 10:28:25 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

> On Thu, 2015-11-19 at 11:11 +0100, Rasmus Villemoes wrote:
>> 
>> First of all, ' ' is one of the characters which hexdump is certainly
>> supposed to spit out, so I think it's better to use some other
>> character for prefilling. Otherwise we wouldn't be able to detect a
>> stray write of a space which wasn't properly guarded by a size
>> check. I'd suggest '\xff' or any other non-ascii
>
> Okay, I may change the ' ' to something, but somehow printable. See
> also below.
>
>> > -			a = r == e && buf[l - 1] == '\0' && buf[l
>> > - 2] == ' ';
>> > -		else
>> > -			a = r == e && buf[50] == '\0' && buf[49]
>> > == '.';
>> > +		memset(test, ' ', sizeof(test));
>> > +		test[sizeof(buf) - 1] = '\0';
>> > +
>> > +		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));
>> 
>> test and buf happen to have the same size, but
>> "test[sizeof(buf) - 1] = '\0'" is rather odd. But you don't even seem
>> to use test in this branch?
>
> Here I feel the test buffer just to print below if any error happens
> when buflen == 0.
>
> That's why I would like to have a somehow printable character.

Oh, but that's wrong! That is, printing the filled test buffer does not
actually show what was "expected", whether you were filling with spaces,
$ or \xff. I really can't think of a situation where you'd want to print
the supposedly unused part of the buffer, so the character used for
filling shouldn't matter.

In any case, the most important thing is to stay away from [ a-f0-9]
which hexdump is certainly expected to produce - the reason I suggested
a non-ascii character was because hexdump might produce any printable
ascii character in the ascii part (but I suppose one could use e.g. '#'
which isn't among the characters we feed it).

>> >  	} else {
>> > -		a = r == e && buf[e] == '\0';
>> > +		int f = min_t(int, e + 1, buflen);
>> > +
>> > +		test_hexdump_prepare_test(len, rs, gs, test,
>> > sizeof(test), ascii);
>> > +		test[f - 1] = '\0';
>> > +
>> > +		a = r == e && !memchr_inv(buf + f, ' ',
>> > sizeof(buf) - f) && !strcmp(buf, test);
>> >  	}
>> 
>> There's also a bit of duplication in the !buflen and buflen
>> branches. Why not pull the computation of f (the number of expected
>> bytes written) outside and do
>
> See above. buflen == 0 is a special case where buffer shouldn't be
> touched at all.

No, buflen==0 is not that special. We expect hexdump to touch exactly
the first buflen bytes (or only e+1, whichever is smaller), and the
remaining bytes should be untouched. A memcmp handles the first part, a
memchr_inv the second.

Rasmus



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/