1. Patchew
  2. QEMU
  3. View series

[PATCH] target/x86: Correctly handle invalid 0x0f 0xc7 0xxx insns

Peter Maydell posted 1 patch 3 weeks, 2 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251021173152.1695997-1-peter.maydell@linaro.org
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
target/i386/tcg/decode-new.c.inc | 2 ++
1 file changed, 2 insertions(+)
[PATCH] target/x86: Correctly handle invalid 0x0f 0xc7 0xxx insns
Posted by Peter Maydell 3 weeks, 2 days ago 
In the decode_group9() function, if we don't recognise the insn as
one that we should handle, we leave the 'entry' pointer unaltered.
Because the X86OpEntry struct has a union for the gen and decode
pointers, this means that the top level code will call decode.e.gen()
which tries to use the decode function pointer (still set to
decode_group9) as a gen function pointer.

This is undefined behaviour, but seems to be mostly harmless in
practice (we call decode_group9() again with bogus arguments and it
does nothing).  If you have CFI enabled then it will trip the CFI
check:

../target/i386/tcg/decode-new.c.inc:2862:9: runtime error: control flow integrity check for type 'void (struct DisasContext *, struct X86DecodedInsn *)' failed during indirect function call

Set *entry to UNKNOWN_OPCODE to provoke the #UD exception, as we do
in decode_group1A() and decode_group11() for similar situations.

Thanks to the bug reporter for the clear description and analysis of
the bug and the simple reproducer.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3172
Fixes: fcd16539ebfe2 ("target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/i386/tcg/decode-new.c.inc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index a50f57dbaab..f4192f10068 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -335,6 +335,8 @@ static void decode_group9(DisasContext *s, CPUX86State *env, X86OpEntry *entry,
         *entry = group9_reg;
     } else if (op == 1) {
         *entry = REX_W(s) ? cmpxchg16b : cmpxchg8b;
+    } else {
+        *entry = UNKNOWN_OPCODE;
     }
 }
 
-- 
2.43.0
Re: [PATCH] target/x86: Correctly handle invalid 0x0f 0xc7 0xxx insns
Posted by Richard Henderson 4 days, 4 hours ago 
On 10/21/25 19:31, Peter Maydell wrote:
> In the decode_group9() function, if we don't recognise the insn as
> one that we should handle, we leave the 'entry' pointer unaltered.
> Because the X86OpEntry struct has a union for the gen and decode
> pointers, this means that the top level code will call decode.e.gen()
> which tries to use the decode function pointer (still set to
> decode_group9) as a gen function pointer.
> 
> This is undefined behaviour, but seems to be mostly harmless in
> practice (we call decode_group9() again with bogus arguments and it
> does nothing).  If you have CFI enabled then it will trip the CFI
> check:
> 
> ../target/i386/tcg/decode-new.c.inc:2862:9: runtime error: control flow integrity check for type 'void (struct DisasContext *, struct X86DecodedInsn *)' failed during indirect function call
> 
> Set *entry to UNKNOWN_OPCODE to provoke the #UD exception, as we do
> in decode_group1A() and decode_group11() for similar situations.
> 
> Thanks to the bug reporter for the clear description and analysis of
> the bug and the simple reproducer.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3172
> Fixes: fcd16539ebfe2 ("target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Queued.

r~
Re: [PATCH] target/x86: Correctly handle invalid 0x0f 0xc7 0xxx insns
Posted by Peter Maydell 1 week, 4 days ago 
Ping: this is reviewed, but would an x86 maintainer like to
pick it up?

thanks
-- PMM

On Tue, 21 Oct 2025 at 18:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the decode_group9() function, if we don't recognise the insn as
> one that we should handle, we leave the 'entry' pointer unaltered.
> Because the X86OpEntry struct has a union for the gen and decode
> pointers, this means that the top level code will call decode.e.gen()
> which tries to use the decode function pointer (still set to
> decode_group9) as a gen function pointer.
>
> This is undefined behaviour, but seems to be mostly harmless in
> practice (we call decode_group9() again with bogus arguments and it
> does nothing).  If you have CFI enabled then it will trip the CFI
> check:
>
> ../target/i386/tcg/decode-new.c.inc:2862:9: runtime error: control flow integrity check for type 'void (struct DisasContext *, struct X86DecodedInsn *)' failed during indirect function call
>
> Set *entry to UNKNOWN_OPCODE to provoke the #UD exception, as we do
> in decode_group1A() and decode_group11() for similar situations.
>
> Thanks to the bug reporter for the clear description and analysis of
> the bug and the simple reproducer.
>
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3172
> Fixes: fcd16539ebfe2 ("target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/i386/tcg/decode-new.c.inc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
> index a50f57dbaab..f4192f10068 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -335,6 +335,8 @@ static void decode_group9(DisasContext *s, CPUX86State *env, X86OpEntry *entry,
>          *entry = group9_reg;
>      } else if (op == 1) {
>          *entry = REX_W(s) ? cmpxchg16b : cmpxchg8b;
> +    } else {
> +        *entry = UNKNOWN_OPCODE;
>      }
>  }
Re: [PATCH] target/x86: Correctly handle invalid 0x0f 0xc7 0xxx insns
Posted by Richard Henderson 3 weeks, 2 days ago 
On 10/21/25 12:31, Peter Maydell wrote:
> In the decode_group9() function, if we don't recognise the insn as
> one that we should handle, we leave the 'entry' pointer unaltered.
> Because the X86OpEntry struct has a union for the gen and decode
> pointers, this means that the top level code will call decode.e.gen()
> which tries to use the decode function pointer (still set to
> decode_group9) as a gen function pointer.
> 
> This is undefined behaviour, but seems to be mostly harmless in
> practice (we call decode_group9() again with bogus arguments and it
> does nothing).  If you have CFI enabled then it will trip the CFI
> check:
> 
> ../target/i386/tcg/decode-new.c.inc:2862:9: runtime error: control flow integrity check for type 'void (struct DisasContext *, struct X86DecodedInsn *)' failed during indirect function call
> 
> Set *entry to UNKNOWN_OPCODE to provoke the #UD exception, as we do
> in decode_group1A() and decode_group11() for similar situations.
> 
> Thanks to the bug reporter for the clear description and analysis of
> the bug and the simple reproducer.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3172
> Fixes: fcd16539ebfe2 ("target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/i386/tcg/decode-new.c.inc | 2 ++

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.