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