lkml.org 
[lkml]   [2011]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] x86: Add a sanity test of x86 decoder

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.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


\
 
 \ /
  Last update: 2011-10-18 08:59    [W:0.129 / U:3.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site