Date:	Tue, 18 Oct 2011 08:54:53 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
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


* 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:

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

> +#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.

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

missing newline.

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

ARRAY_SIZE().

> +	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?

> +{
> +	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.

> +#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?

> +	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?

> +		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 ?

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

Thanks,

	Ingo
--
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/

Powered by blists - more mailing lists