diff mbox series

[v9,27/27] KVM: x86: Stop emulating for CET protected branch instructions

Message ID 20240124024200.102792-28-weijiang.yang@intel.com
State New
Headers
Series Enable CET Virtualization |

Commit Message

Yang, Weijiang Jan. 24, 2024, 2:42 a.m. UTC 
  Don't emulate the branch instructions, e.g., CALL/RET/JMP etc., when CET
is active in guest, return KVM_INTERNAL_ERROR_EMULATION to userspace to
handle it.

KVM doesn't emulate CPU behaviors to check CET protected stuffs while
emulating guest instructions, instead it stops emulation on detecting
the instructions in process are CET protected. By doing so, it can avoid
generating bogus #CP in guest and preventing CET protected execution flow
subversion from guest side.

Suggested-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/emulate.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Comments

Chao Gao Jan. 26, 2024, 8:53 a.m. UTC | #1 
On Tue, Jan 23, 2024 at 06:42:00PM -0800, Yang Weijiang wrote:
>Don't emulate the branch instructions, e.g., CALL/RET/JMP etc., when CET
>is active in guest, return KVM_INTERNAL_ERROR_EMULATION to userspace to
>handle it.
>
>KVM doesn't emulate CPU behaviors to check CET protected stuffs while
>emulating guest instructions, instead it stops emulation on detecting
>the instructions in process are CET protected. By doing so, it can avoid
>generating bogus #CP in guest and preventing CET protected execution flow
>subversion from guest side.
>
>Suggested-by: Chao Gao <chao.gao@intel.com>
>Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>---
> arch/x86/kvm/emulate.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
>diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>index e223043ef5b2..ad15ce055a1d 100644
>--- a/arch/x86/kvm/emulate.c
>+++ b/arch/x86/kvm/emulate.c
>@@ -178,6 +178,7 @@
> #define IncSP       ((u64)1 << 54)  /* SP is incremented before ModRM calc */
> #define TwoMemOp    ((u64)1 << 55)  /* Instruction has two memory operand */
> #define IsBranch    ((u64)1 << 56)  /* Instruction is considered a branch. */
>+#define IsProtected ((u64)1 << 57)  /* Instruction is protected by CET. */

the name IsProtected doesn't seem clear to me. Its meaning isn't obvious from
the name and may be confused with protected mode. Maybe we can add two flags:
"IndirectBranch" and "ShadowStack".

> 
> #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
> 
>@@ -4098,9 +4099,9 @@ static const struct opcode group4[] = {
> static const struct opcode group5[] = {
> 	F(DstMem | SrcNone | Lock,		em_inc),
> 	F(DstMem | SrcNone | Lock,		em_dec),
>-	I(SrcMem | NearBranch | IsBranch,       em_call_near_abs),
>-	I(SrcMemFAddr | ImplicitOps | IsBranch, em_call_far),
>-	I(SrcMem | NearBranch | IsBranch,       em_jmp_abs),
>+	I(SrcMem | NearBranch | IsBranch | IsProtected, em_call_near_abs),
>+	I(SrcMemFAddr | ImplicitOps | IsBranch | IsProtected, em_call_far),
>+	I(SrcMem | NearBranch | IsBranch | IsProtected, em_jmp_abs),

In SDM, I don't see a list of instructions that are affected by CET. how do you
get the list.
Yang, Weijiang Jan. 26, 2024, 12:56 p.m. UTC | #2 
On 1/26/2024 4:53 PM, Chao Gao wrote:
> On Tue, Jan 23, 2024 at 06:42:00PM -0800, Yang Weijiang wrote:
>> Don't emulate the branch instructions, e.g., CALL/RET/JMP etc., when CET
>> is active in guest, return KVM_INTERNAL_ERROR_EMULATION to userspace to
>> handle it.
>>
>> KVM doesn't emulate CPU behaviors to check CET protected stuffs while
>> emulating guest instructions, instead it stops emulation on detecting
>> the instructions in process are CET protected. By doing so, it can avoid
>> generating bogus #CP in guest and preventing CET protected execution flow
>> subversion from guest side.
>>
>> Suggested-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>> arch/x86/kvm/emulate.c | 27 ++++++++++++++++-----------
>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index e223043ef5b2..ad15ce055a1d 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -178,6 +178,7 @@
>> #define IncSP       ((u64)1 << 54)  /* SP is incremented before ModRM calc */
>> #define TwoMemOp    ((u64)1 << 55)  /* Instruction has two memory operand */
>> #define IsBranch    ((u64)1 << 56)  /* Instruction is considered a branch. */
>> +#define IsProtected ((u64)1 << 57)  /* Instruction is protected by CET. */
> the name IsProtected doesn't seem clear to me. Its meaning isn't obvious from
> the name and may be confused with protected mode. Maybe we can add two flags:
> "IndirectBranch" and "ShadowStack".

Hmm, maybe it's worth to distinguish specific instruction protection type against current CET
enabling status. Let me double check.

>> #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
>>
>> @@ -4098,9 +4099,9 @@ static const struct opcode group4[] = {
>> static const struct opcode group5[] = {
>> 	F(DstMem | SrcNone | Lock,		em_inc),
>> 	F(DstMem | SrcNone | Lock,		em_dec),
>> -	I(SrcMem | NearBranch | IsBranch,       em_call_near_abs),
>> -	I(SrcMemFAddr | ImplicitOps | IsBranch, em_call_far),
>> -	I(SrcMem | NearBranch | IsBranch,       em_jmp_abs),
>> +	I(SrcMem | NearBranch | IsBranch | IsProtected, em_call_near_abs),
>> +	I(SrcMemFAddr | ImplicitOps | IsBranch | IsProtected, em_call_far),
>> +	I(SrcMem | NearBranch | IsBranch | IsProtected, em_jmp_abs),
> In SDM, I don't see a list of instructions that are affected by CET. how do you
> get the list.

In SDM Vol. 1/17.2 and 17.3, and Vol.2 instruction references on branch instructions.
diff mbox series

Patch

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e223043ef5b2..ad15ce055a1d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -178,6 +178,7 @@ 
 #define IncSP       ((u64)1 << 54)  /* SP is incremented before ModRM calc */
 #define TwoMemOp    ((u64)1 << 55)  /* Instruction has two memory operand */
 #define IsBranch    ((u64)1 << 56)  /* Instruction is considered a branch. */
+#define IsProtected ((u64)1 << 57)  /* Instruction is protected by CET. */
 
 #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
 
@@ -4098,9 +4099,9 @@  static const struct opcode group4[] = {
 static const struct opcode group5[] = {
 	F(DstMem | SrcNone | Lock,		em_inc),
 	F(DstMem | SrcNone | Lock,		em_dec),
-	I(SrcMem | NearBranch | IsBranch,       em_call_near_abs),
-	I(SrcMemFAddr | ImplicitOps | IsBranch, em_call_far),
-	I(SrcMem | NearBranch | IsBranch,       em_jmp_abs),
+	I(SrcMem | NearBranch | IsBranch | IsProtected, em_call_near_abs),
+	I(SrcMemFAddr | ImplicitOps | IsBranch | IsProtected, em_call_far),
+	I(SrcMem | NearBranch | IsBranch | IsProtected, em_jmp_abs),
 	I(SrcMemFAddr | ImplicitOps | IsBranch, em_jmp_far),
 	I(SrcMem | Stack | TwoMemOp,		em_push), D(Undefined),
 };
@@ -4362,11 +4363,11 @@  static const struct opcode opcode_table[256] = {
 	/* 0xC8 - 0xCF */
 	I(Stack | SrcImmU16 | Src2ImmByte | IsBranch, em_enter),
 	I(Stack | IsBranch, em_leave),
-	I(ImplicitOps | SrcImmU16 | IsBranch, em_ret_far_imm),
-	I(ImplicitOps | IsBranch, em_ret_far),
-	D(ImplicitOps | IsBranch), DI(SrcImmByte | IsBranch, intn),
+	I(ImplicitOps | SrcImmU16 | IsBranch | IsProtected, em_ret_far_imm),
+	I(ImplicitOps | IsBranch | IsProtected, em_ret_far),
+	D(ImplicitOps | IsBranch), DI(SrcImmByte | IsBranch | IsProtected, intn),
 	D(ImplicitOps | No64 | IsBranch),
-	II(ImplicitOps | IsBranch, em_iret, iret),
+	II(ImplicitOps | IsBranch | IsProtected, em_iret, iret),
 	/* 0xD0 - 0xD7 */
 	G(Src2One | ByteOp, group2), G(Src2One, group2),
 	G(Src2CL | ByteOp, group2), G(Src2CL, group2),
@@ -4382,7 +4383,7 @@  static const struct opcode opcode_table[256] = {
 	I2bvIP(SrcImmUByte | DstAcc, em_in,  in,  check_perm_in),
 	I2bvIP(SrcAcc | DstImmUByte, em_out, out, check_perm_out),
 	/* 0xE8 - 0xEF */
-	I(SrcImm | NearBranch | IsBranch, em_call),
+	I(SrcImm | NearBranch | IsBranch | IsProtected, em_call),
 	D(SrcImm | ImplicitOps | NearBranch | IsBranch),
 	I(SrcImmFAddr | No64 | IsBranch, em_jmp_far),
 	D(SrcImmByte | ImplicitOps | NearBranch | IsBranch),
@@ -4401,7 +4402,7 @@  static const struct opcode opcode_table[256] = {
 static const struct opcode twobyte_table[256] = {
 	/* 0x00 - 0x0F */
 	G(0, group6), GD(0, &group7), N, N,
-	N, I(ImplicitOps | EmulateOnUD | IsBranch, em_syscall),
+	N, I(ImplicitOps | EmulateOnUD | IsBranch | IsProtected, em_syscall),
 	II(ImplicitOps | Priv, em_clts, clts), N,
 	DI(ImplicitOps | Priv, invd), DI(ImplicitOps | Priv, wbinvd), N, N,
 	N, D(ImplicitOps | ModRM | SrcMem | NoAccess), N, N,
@@ -4432,8 +4433,8 @@  static const struct opcode twobyte_table[256] = {
 	IIP(ImplicitOps, em_rdtsc, rdtsc, check_rdtsc),
 	II(ImplicitOps | Priv, em_rdmsr, rdmsr),
 	IIP(ImplicitOps, em_rdpmc, rdpmc, check_rdpmc),
-	I(ImplicitOps | EmulateOnUD | IsBranch, em_sysenter),
-	I(ImplicitOps | Priv | EmulateOnUD | IsBranch, em_sysexit),
+	I(ImplicitOps | EmulateOnUD | IsBranch | IsProtected, em_sysenter),
+	I(ImplicitOps | Priv | EmulateOnUD | IsBranch | IsProtected, em_sysexit),
 	N, N,
 	N, N, N, N, N, N, N, N,
 	/* 0x40 - 0x4F */
@@ -4971,6 +4972,10 @@  int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
 	if (ctxt->d == 0)
 		return EMULATION_FAILED;
 
+	if ((opcode.flags & IsProtected) &&
+	    (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_CET))
+		return EMULATION_FAILED;
+
 	ctxt->execute = opcode.u.execute;
 
 	if (unlikely(emulation_type & EMULTYPE_TRAP_UD) &&