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