Re: [PATCH v5 05/16] hvf: Fix OOB write in RDTSCP instruction decode

[Peter Maydell]

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