linux-kernel - Re: [RFC PATCH] x86: Add a sanity test of x86 decoder

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:	Wed, 19 Oct 2011 13:29:32 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Andi Kleen <andi@...stfloor.org>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org, acme@...hat.com,
	ming.m.lin@...el.com, robert.richter@....com, ravitillo@....gov,
	yrl.pp-manager.tt@...achi.com,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [RFC PATCH] x86: Add a sanity test of x86 decoder

(2011/10/18 15:54), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@...achi.com> wrote:
> 
>> Add a sanity test of x86 insn decoder against the random
>> input. This test is also able to reproduce the bug by
>> passing random-seed and iteration-number, or by passing
>> input while which has invalid byte code.
> 
> Looks good in general.
> 
> a few nitpicking details:

Thank you for the comments :)

> 
>> -posttest: $(obj)/test_get_len vmlinux
>> +quiet_cmd_sanitytest = TEST    $@
>> +      cmd_sanitytest = $(obj)/insn_sanity $(posttest_64bit) -m 1000000
> 
> Just curious, what's the execution time for this? I'd expect 
> milliseconds, but there will also be urandom overhead.

I measured it with time command.
---
Succeed: decoded and checked 1000000 insns (seed:0x73f2b3bb)

real    0m0.152s
user    0m0.149s
sys     0m0.002s
---

Here, you can see how long it takes. It actually refers /dev/urandom
just one time at start, so I guess there is no urandom overhead.

>> +#define unlikely(cond) (cond)
>> +
>> +#include <asm/insn.h>
>> +#include <inat.c>
>> +#include <insn.c>
>> +
>> +/*
>> + * Test of instruction analysis against tampering.
>> + * Feed random binary to instruction decoder and ensure not to
>> + * access out-of-instruction-buffer.
>> + */
>> +
>> +#define DEFAULT_MAX_ITER	10000
>> +#define INSN_NOP 0x90
>> +
>> +static const char *prog;
>> +static int verbose;
>> +static int x86_64;
>> +static unsigned int seed;
>> +static unsigned long nr_iter;
>> +static unsigned long max_iter = DEFAULT_MAX_ITER;
>> +static char insn_buf[MAX_INSN_SIZE * 2];
>> +static FILE *input_file;
> 
> This needs more comments and a bit more vertical structure.

OK.

>> +static void dump_stream(FILE *fp, const char *msg, struct insn *insn)
>> +{
>> +	int i;
>> +	fprintf(fp, "%s:\n code:", msg);
> 
> missing newline.

This prints a header of code sequence, so that we'll see a
message like below;

Error message:
code: 00 01 02 03 04 ...

>> +static int get_next_insn(void)
>> +{
>> +	char buf[256]  = "", *tmp;
>> +	int i;
>> +
>> +	tmp = fgets(buf, 256, input_file);
> 
> ARRAY_SIZE().

OK.

>> +	if (tmp == NULL || feof(input_file))
>> +		return 0;
>> +
>> +	for (i = 0; i < MAX_INSN_SIZE; i++) {
>> +		((unsigned char *)insn_buf)[i] = (unsigned char)strtoul(tmp, &tmp, 16);
> 
> why is this cast needed? Shouldnt insn_buf[] have this type if this 
> is how it's used?

Yes, the type of insn_buf can be changed.

> 
>> +{
>> +	int i;
>> +	if (nr_iter >= max_iter)
> 
> missing newline.
> 
>> +	for (i = 0; i < MAX_INSN_SIZE; i += 2)
>> +		*(unsigned short *)(&insn_buf[i]) = random() & 0xffff;
> 
> this silently assumes that MAX_INSN_SIZE is a multiple of 2. Which is 
> true ... for now.

Ah, right. OK, add a line to fill the last byte if needed.

>> +#define insn_complete(insn)	\
>> +	(insn.opcode.got && insn.modrm.got && insn.sib.got && \
>> +	 insn.displacement.got && insn.immediate.got)
> 
> This could move into insn.h (even though only user-space uses it), 
> right?

Right.

> 
>> +	while (generate_random_insn()) {
> 
> 
> this loop is really weird: half of it is hidden in 
> generate_random_insn()'s use of nr_iter global variable!
> 
> Why not just do it in the old-fashioned way:
> 
> 	for (i = 0; i < max_iter; i++) {
> 		...
> 	}
> 
> and keep generate_random_insn() loop-invariant?

OK, I'll change this.

> 
>> +		if (insn.next_byte <= insn.kaddr ||
>> +		    insn.kaddr + MAX_INSN_SIZE < insn.next_byte) {
>> +			/* Access out-of-range memory */
>> +			dump_stream(stdout, "Find access violation", &insn);
>> +			warnings++;
> 
> s/Find/Found ?
> 
>> +	if (warnings)
>> +		fprintf(stdout, "Warning: decoded and checked %d insns with %d warnings (seed:0x%x)\n", insns, warnings, seed);
>> +	else
>> +		fprintf(stdout, "Succeed: decoded and checked %d insns (seed:0x%x)\n", insns, seed);
> 
> s/Succeed/Success ?

Oops...

> Also, s/insns/random instructions - there's rarely any good reason to 
> abbreviate in human readable output.

Agreed.

Thank you!

> 
> Thanks,
> 
> 	Ingo


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com
--
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/