| Message ID | 20220214185605.28087-6-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | host: Support macOS 12 | expand |
On Mon, 14 Feb 2022 at 18:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > From: Cameron Esfahani <dirty@apple.com> > > A guest could craft a specific stream of instructions that will have QEMU > write 0xF9 to inappropriate locations in memory. Add additional asserts > to check for this. Generate a #UD if there are more than 14 prefix bytes. > > Found by Julian Stecklina <julian.stecklina@cyberus-technology.de> > > Signed-off-by: Cameron Esfahani <dirty@apple.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/i386/hvf/x86_decode.c | 11 +++++++++-- > target/i386/hvf/x86hvf.c | 8 ++++++++ > target/i386/hvf/x86hvf.h | 1 + > 3 files changed, 18 insertions(+), 2 deletions(-) I'm not a huge fan of the VM_PANIC_ON() macro, which appears to be essentially an obfuscated assert(), but it's the existing code style in this decoder I guess. > @@ -1847,7 +1849,8 @@ void calc_modrm_operand(CPUX86State *env, struct x86_decode *decode, > > static void decode_prefix(CPUX86State *env, struct x86_decode *decode) > { > - while (1) { > + /* At most 14 prefix bytes. */ > + for (int i = 0; i < 14; i++) { > /* > * REX prefix must come after legacy prefixes. > * REX before legacy is ignored. > @@ -1892,6 +1895,8 @@ static void decode_prefix(CPUX86State *env, struct x86_decode *decode) > return; > } > } > + /* Too many prefixes! Generate #UD. */ > + hvf_inject_ud(env); This doesn't look right. This is the decoder, so it shouldn't be actively affecting the state of the CPU. Also, we don't do anything here to cause us to stop trying to decode this instruction, so we'll carry on through the rest of decode and then the caller will call exec_instruction() on whatever results. I think if you want to say "this instruction should cause a #UD" you should emit an X86_DECODE_CMD_* for "raise #UD", stop decode of the insn at that point, and then handle that in exec_instruction(). You probably also need to take special care to avoid tripping the assert(ins_len == decode.len) at one of the callsites in hvf.c (or else just drop or modify that assert). thanks -- PMM
diff --git a/target/i386/hvf/x86_decode.c b/target/i386/hvf/x86_decode.c index 062713b1a4..fbaf1813e8 100644 --- a/target/i386/hvf/x86_decode.c +++ b/target/i386/hvf/x86_decode.c @@ -24,6 +24,7 @@ #include "vmx.h" #include "x86_mmu.h" #include "x86_descr.h" +#include "x86hvf.h" #define OPCODE_ESCAPE 0xf @@ -541,7 +542,8 @@ static void decode_lidtgroup(CPUX86State *env, struct x86_decode *decode) }; decode->cmd = group[decode->modrm.reg]; if (0xf9 == decode->modrm.modrm) { - decode->opcode[decode->len++] = decode->modrm.modrm; + VM_PANIC_ON(decode->opcode_len >= sizeof(decode->opcode)); + decode->opcode[decode->opcode_len++] = decode->modrm.modrm; decode->cmd = X86_DECODE_CMD_RDTSCP; } } @@ -1847,7 +1849,8 @@ void calc_modrm_operand(CPUX86State *env, struct x86_decode *decode, static void decode_prefix(CPUX86State *env, struct x86_decode *decode) { - while (1) { + /* At most 14 prefix bytes. */ + for (int i = 0; i < 14; i++) { /* * REX prefix must come after legacy prefixes. * REX before legacy is ignored. @@ -1892,6 +1895,8 @@ static void decode_prefix(CPUX86State *env, struct x86_decode *decode) return; } } + /* Too many prefixes! Generate #UD. */ + hvf_inject_ud(env); } void set_addressing_size(CPUX86State *env, struct x86_decode *decode) @@ -2090,11 +2095,13 @@ static void decode_opcodes(CPUX86State *env, struct x86_decode *decode) uint8_t opcode; opcode = decode_byte(env, decode); + VM_PANIC_ON(decode->opcode_len >= sizeof(decode->opcode)); decode->opcode[decode->opcode_len++] = opcode; if (opcode != OPCODE_ESCAPE) { decode_opcode_1(env, decode, opcode); } else { opcode = decode_byte(env, decode); + VM_PANIC_ON(decode->opcode_len >= sizeof(decode->opcode)); decode->opcode[decode->opcode_len++] = opcode; decode_opcode_2(env, decode, opcode); } diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c index 05ec1bddc4..a805c125d9 100644 --- a/target/i386/hvf/x86hvf.c +++ b/target/i386/hvf/x86hvf.c @@ -425,6 +425,14 @@ bool hvf_inject_interrupts(CPUState *cpu_state) & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)); } +void hvf_inject_ud(CPUX86State *env) +{ + env->exception_nr = EXCP06_ILLOP; + env->exception_injected = 1; + env->has_error_code = false; + env->error_code = 0; +} + int hvf_process_events(CPUState *cpu_state) { X86CPU *cpu = X86_CPU(cpu_state); diff --git a/target/i386/hvf/x86hvf.h b/target/i386/hvf/x86hvf.h index 99ed8d608d..ef472a32f9 100644 --- a/target/i386/hvf/x86hvf.h +++ b/target/i386/hvf/x86hvf.h @@ -22,6 +22,7 @@ int hvf_process_events(CPUState *); bool hvf_inject_interrupts(CPUState *); +void hvf_inject_ud(CPUX86State *); void hvf_set_segment(struct CPUState *cpu, struct vmx_segment *vmx_seg, SegmentCache *qseg, bool is_tr); void hvf_get_segment(SegmentCache *qseg, struct vmx_segment *vmx_seg);