| Message ID | 20220306231753.50277-7-philippe.mathieu.daude@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | host: Support macOS 12 | expand |
Hi Paolo, I forgot to Cc you. Could you Ack this patch? On 7/3/22 00:17, Philippe Mathieu-Daudé 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> > Message-Id: <20220219063831.35356-1-dirty@apple.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/i386/hvf/x86_decode.c | 12 ++++++++++-- > target/i386/hvf/x86hvf.c | 8 ++++++++ > target/i386/hvf/x86hvf.h | 1 + > 3 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/target/i386/hvf/x86_decode.c b/target/i386/hvf/x86_decode.c > index 062713b1a4..5d051252b4 100644 > --- a/target/i386/hvf/x86_decode.c > +++ b/target/i386/hvf/x86_decode.c > @@ -24,8 +24,10 @@ > #include "vmx.h" > #include "x86_mmu.h" > #include "x86_descr.h" > +#include "x86hvf.h" > > #define OPCODE_ESCAPE 0xf > +#define X86_MAX_INSN_PREFIX_LENGTH (14) > > static void decode_invalid(CPUX86State *env, struct x86_decode *decode) > { > @@ -541,7 +543,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 +1850,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 X86_MAX_INSN_PREFIX_LENGTH prefix bytes. */ > + for (int i = 0; i < X86_MAX_INSN_PREFIX_LENGTH; i++) { > /* > * REX prefix must come after legacy prefixes. > * REX before legacy is ignored. > @@ -1892,6 +1896,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 +2096,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 bec9fc5814..a338c207b7 100644 > --- a/target/i386/hvf/x86hvf.c > +++ b/target/i386/hvf/x86hvf.c > @@ -423,6 +423,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 db6003d6bd..427cdc1c13 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);
On Wed, 9 Mar 2022 at 10:20, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > Hi Paolo, > > I forgot to Cc you. Could you Ack this patch? I had review comments on the version of this patch in v5 which still need to be addressed: https://lore.kernel.org/qemu-devel/CAFEAcA8yaBOD3KXc-DY94oqzC5wkCENPkePgVCybqR=9NmdQFQ@mail.gmail.com/ thanks -- PMM
On 15/3/22 13:58, Peter Maydell wrote: > On Wed, 9 Mar 2022 at 10:20, Philippe Mathieu-Daudé > <philippe.mathieu.daude@gmail.com> wrote: >> >> Hi Paolo, >> >> I forgot to Cc you. Could you Ack this patch? > > I had review comments on the version of this patch in v5 which > still need to be addressed: > > https://lore.kernel.org/qemu-devel/CAFEAcA8yaBOD3KXc-DY94oqzC5wkCENPkePgVCybqR=9NmdQFQ@mail.gmail.com/ Thanks for pointing at your comments, I totally missed them. I'll let Cameron address them. Regards, Phil.
diff --git a/target/i386/hvf/x86_decode.c b/target/i386/hvf/x86_decode.c index 062713b1a4..5d051252b4 100644 --- a/target/i386/hvf/x86_decode.c +++ b/target/i386/hvf/x86_decode.c @@ -24,8 +24,10 @@ #include "vmx.h" #include "x86_mmu.h" #include "x86_descr.h" +#include "x86hvf.h" #define OPCODE_ESCAPE 0xf +#define X86_MAX_INSN_PREFIX_LENGTH (14) static void decode_invalid(CPUX86State *env, struct x86_decode *decode) { @@ -541,7 +543,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 +1850,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 X86_MAX_INSN_PREFIX_LENGTH prefix bytes. */ + for (int i = 0; i < X86_MAX_INSN_PREFIX_LENGTH; i++) { /* * REX prefix must come after legacy prefixes. * REX before legacy is ignored. @@ -1892,6 +1896,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 +2096,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 bec9fc5814..a338c207b7 100644 --- a/target/i386/hvf/x86hvf.c +++ b/target/i386/hvf/x86hvf.c @@ -423,6 +423,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 db6003d6bd..427cdc1c13 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);