Re: [PATCH v7 06/22] hvf: Fix OOB write in RDTSCP instruction decode

[Philippe Mathieu-Daudé]

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