diff mbox series

KVM: x86: Fix the #GP(0) and #UD conditions for XSETBV emulation

Message ID 20220117072456.71155-1-likexu@tencent.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86: Fix the #GP(0) and #UD conditions for XSETBV emulation | expand

Commit Message

Like Xu Jan. 17, 2022, 7:24 a.m. UTC 
From: Like Xu <likexu@tencent.com>

According to Intel SDM "XSETBV—Set Extended Control Register",
the #UD exception is raised if CPUID.01H:ECX.XSAVE[bit 26] = 0
or if CR4.OSXSAVE[bit 18] = 0, and the #GP(0) exception for the
reason "if the current privilege level is not 0" is only valid
in the protected mode. Translate them into KVM terms.

Fixes: 92f9895c146d ("Move XSETBV emulation to common code")
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/x86.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini Jan. 17, 2022, 8:31 a.m. UTC | #1 
On 1/17/22 08:24, Like Xu wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 76b4803dd3bd..7d8622e592bb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1024,7 +1024,11 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>   
>   int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
>   {
> -	if (static_call(kvm_x86_get_cpl)(vcpu) != 0 ||
> +	if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) ||
> +	    !kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE))
> +		return kvm_handle_invalid_op(vcpu);

There's no need to check XSAVE, because it XSAVE=0 will prevent setting 
CR4.OSXSAVE.

Likewise, CPL and SS.DPL are also defined in real mode so there's no 
need to check is_protmode.  The Intel manuals sometimes still act as the 
descriptor caches don't exist, even though VMX effectively made them 
part of the architecture.

Also, the "Fixes" tag is not really correct as the behavior was the same 
before.  Rather, it fixes commit 02d4160fbd76 ("x86: KVM: add xsetbv to 
the emulator", 2019-08-22).  Checking OSXSAVE is a bug in the emulator 
path, even though it's not needed in the XSETBV vmexit case.

Thanks,

Paolo

> +	if ((is_protmode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) != 0) ||
>   	    __kvm_set_xcr(vcpu, kvm_rcx_read(vcpu), kvm_read_edx_eax(vcpu))) {
>   		kvm_inject_gp(vcpu, 0);
>   		return 1;
Like Xu Jan. 17, 2022, 9:44 a.m. UTC | #2 
Uh, thanks for your prompt response.

On 17/1/2022 4:31 pm, Paolo Bonzini wrote:
> On 1/17/22 08:24, Like Xu wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 76b4803dd3bd..7d8622e592bb 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1024,7 +1024,11 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 
>> index, u64 xcr)
>>   int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
>>   {
>> -    if (static_call(kvm_x86_get_cpl)(vcpu) != 0 ||
>> +    if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) ||
>> +        !kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE))
>> +        return kvm_handle_invalid_op(vcpu);
> 
> There's no need to check XSAVE, because it XSAVE=0 will prevent setting 
> CR4.OSXSAVE.

So we just need to check X86_CR4_OSXSAVE for #UD here ?

> 
> Likewise, CPL and SS.DPL are also defined in real mode so there's no need to 
> check is_protmode.  The Intel manuals sometimes still act as the descriptor 
> caches don't exist, even though VMX effectively made them part of the architecture.

OK, make sense to drop is_protmode().

> 
> Also, the "Fixes" tag is not really correct as the behavior was the same 
> before.  Rather, it fixes commit 02d4160fbd76 ("x86: KVM: add xsetbv to the 

It seems the original code comes from 81dd35d42c9a ("KVM: SVM: Add xsetbv 
intercept").
2acf923e38 ("KVM: VMX: Enable XSAVE/XRSTOR for guest") and 92f9895c146d.

> emulator", 2019-08-22).  Checking OSXSAVE is a bug in the emulator path, even 
> though it's not needed in the XSETBV vmexit case.

The kvm_emulate_xsetbv() has been removed from the emulator path.
I'm not really sure why it's not needed in the XSETBV vmexit case. More details ?

> 
> Thanks,
> 
> Paolo
> 
>> +    if ((is_protmode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) != 0) ||
>>           __kvm_set_xcr(vcpu, kvm_rcx_read(vcpu), kvm_read_edx_eax(vcpu))) {
>>           kvm_inject_gp(vcpu, 0);
>>           return 1;
> 
>
Paolo Bonzini Jan. 17, 2022, 11:18 a.m. UTC | #3 
On 1/17/22 10:44, Like Xu wrote:
>> Also, the "Fixes" tag is not really correct as the behavior was the 
>> same before.  Rather, it fixes commit 02d4160fbd76 ("x86: KVM: add 
>> xsetbv to the 
> 
> It seems the original code comes from 81dd35d42c9a ("KVM: SVM: Add 
> xsetbv intercept").
> 2acf923e38 ("KVM: VMX: Enable XSAVE/XRSTOR for guest") and 92f9895c146d.
> 
>> emulator", 2019-08-22).  Checking OSXSAVE is a bug in the emulator 
>> path, even though it's not needed in the XSETBV vmexit case.
> 
> The kvm_emulate_xsetbv() has been removed from the emulator path.
> I'm not really sure why it's not needed in the XSETBV vmexit case. More 
> details ?

Nevermind, I confused AMD (where #UD is generated before checking for 
exceptions) with Intel where it's unconditional.

So the bug should be there since 2acf923e38 by executing XSETBV with 
CR4.XSAVE=0.  If so, please include a testcase.

Paolo

>>
>> Thanks,
>>
>> Paolo
>>
>>> +    if ((is_protmode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) != 
>>> 0) ||
>>>           __kvm_set_xcr(vcpu, kvm_rcx_read(vcpu), 
>>> kvm_read_edx_eax(vcpu))) {
>>>           kvm_inject_gp(vcpu, 0);
>>>           return 1;
>>
>>
>
Like Xu Jan. 20, 2022, 7:48 a.m. UTC | #4 
On 17/1/2022 7:18 pm, Paolo Bonzini wrote:
> On 1/17/22 10:44, Like Xu wrote:
>>> Also, the "Fixes" tag is not really correct as the behavior was the same 
>>> before.  Rather, it fixes commit 02d4160fbd76 ("x86: KVM: add xsetbv to the 
>>
>> It seems the original code comes from 81dd35d42c9a ("KVM: SVM: Add xsetbv 
>> intercept").
>> 2acf923e38 ("KVM: VMX: Enable XSAVE/XRSTOR for guest") and 92f9895c146d.
>>
>>> emulator", 2019-08-22).  Checking OSXSAVE is a bug in the emulator path, even 
>>> though it's not needed in the XSETBV vmexit case.
>>
>> The kvm_emulate_xsetbv() has been removed from the emulator path.
>> I'm not really sure why it's not needed in the XSETBV vmexit case. More details ?
> 
> Nevermind, I confused AMD (where #UD is generated before checking for 
> exceptions) with Intel where it's unconditional.
> 
> So the bug should be there since 2acf923e38 by executing XSETBV with 
> CR4.XSAVE=0.  If so, please include a testcase.

In the testcase "executing XSETBV with CR4.XSAVE=0",

- on the VMX, #UD delivery does not require vm-exit;
- on the SVM, #UD is trapped but goes to the ud_interception() path;

There is no room to run this pointless patch.
Thank you and sorry for the noise.

> 
> Paolo
> 
>>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>>> +    if ((is_protmode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) != 0) ||
>>>>           __kvm_set_xcr(vcpu, kvm_rcx_read(vcpu), kvm_read_edx_eax(vcpu))) {
>>>>           kvm_inject_gp(vcpu, 0);
>>>>           return 1;
>>>
>>>
>>
> 
>
Paolo Bonzini Jan. 20, 2022, 9:17 a.m. UTC | #5 
On 1/20/22 08:48, Like Xu wrote:
> 
> In the testcase "executing XSETBV with CR4.XSAVE=0",
> 
> - on the VMX, #UD delivery does not require vm-exit;

Not your fault, it would be nicer if the Intel manual told the truth;
it says: "The following instructions cause VM exits when they are
executed in VMX non-root operation: CPUID, GETSEC[1], INVD, and XSETBV."

Footnote [1] says "An execution of GETSEC causes an invalid-opcode
exception (#UD) if CR4.SMXE[Bit 14] = 0", and there is no such footnote
for XSETBV.  Nevertheless, when tracing xsave.flat, I see that there's
a #UD vmexit and not an XSETBV vmexit:

         qemu-kvm-1637698 [019] 758186.750321: kvm_entry:            vcpu 0, rip 0x4028b7
         qemu-kvm-1637698 [019] 758186.750322: kvm_exit:             vcpu 0 reason EXCEPTION_NMI rip 0x40048d info1 0x0000000000000000 info2 0x0000000000000000 intr_info 0x80000306 error_code 0x00000000
         qemu-kvm-1637698 [019] 758186.750324: kvm_emulate_insn:     0:40048d:0f 01 d1 (prot64)
         qemu-kvm-1637698 [019] 758186.750325: kvm_inj_exception:    #UD (0x0)

So while my gut feeling that #UD would not cause a vmexit was correct,
technically I was reading the SDM incorrectly.

Jun, can you have this fixed?

Paolo

> - on the SVM, #UD is trapped but goes to the ud_interception() path;
Xiaoyao Li Jan. 20, 2022, 9:31 a.m. UTC | #6 
On 1/20/2022 5:17 PM, Paolo Bonzini wrote:
> On 1/20/22 08:48, Like Xu wrote:
>>
>> In the testcase "executing XSETBV with CR4.XSAVE=0",
>>
>> - on the VMX, #UD delivery does not require vm-exit;
> 
> Not your fault, it would be nicer if the Intel manual told the truth;
> it says: "The following instructions cause VM exits when they are
> executed in VMX non-root operation: CPUID, GETSEC[1], INVD, and XSETBV."
> 
> Footnote [1] says "An execution of GETSEC causes an invalid-opcode
> exception (#UD) if CR4.SMXE[Bit 14] = 0", and there is no such footnote
> for XSETBV.  Nevertheless, when tracing xsave.flat, I see that there's
> a #UD vmexit and not an XSETBV vmexit:
> 
>          qemu-kvm-1637698 [019] 758186.750321: kvm_entry:            
> vcpu 0, rip 0x4028b7
>          qemu-kvm-1637698 [019] 758186.750322: kvm_exit:             
> vcpu 0 reason EXCEPTION_NMI rip 0x40048d info1 0x0000000000000000 info2 
> 0x0000000000000000 intr_info 0x80000306 error_code 0x00000000
>          qemu-kvm-1637698 [019] 758186.750324: kvm_emulate_insn:     
> 0:40048d:0f 01 d1 (prot64)
>          qemu-kvm-1637698 [019] 758186.750325: kvm_inj_exception:    #UD 
> (0x0)
> 
> So while my gut feeling that #UD would not cause a vmexit was correct,
> technically I was reading the SDM incorrectly.

SDM also states

   Certain exceptions have priority over VM exits. These include
   invalid-opcode exception, faults based on privilege level,
   and general-protection exceptions that are based on checking
   I/O permission bits in the task-state segment(TSS)

in "Relative Priority of Faults and VM Exits"

So my understanding is that the architectural check always takes the 
higher priority than VM exit.

> Jun, can you have this fixed?
> 
> Paolo
> 
>> - on the SVM, #UD is trapped but goes to the ud_interception() path;
>
Paolo Bonzini Jan. 20, 2022, 9:49 a.m. UTC | #7 
On 1/20/22 10:31, Xiaoyao Li wrote:
>>
>> So while my gut feeling that #UD would not cause a vmexit was correct,
>> technically I was reading the SDM incorrectly.
> 
> SDM also states
> 
>    Certain exceptions have priority over VM exits. These include
>    invalid-opcode exception, faults based on privilege level,
>    and general-protection exceptions that are based on checking
>    I/O permission bits in the task-state segment(TSS)
> 
> in "Relative Priority of Faults and VM Exits"
> 
> So my understanding is that the architectural check always takes the 
> higher priority than VM exit.

Good point!  It's right above in 25.1.1.  I was confused by the specific 
mention of GETSEC, but the reason for the footnote is because undefined 
GETSEC leaves cause a vmexit instead of #UD, and GETSEC vmexits also 
override #GP faults based on privilege level.

Thanks,

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 76b4803dd3bd..7d8622e592bb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1024,7 +1024,11 @@  static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 
 int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
 {
-	if (static_call(kvm_x86_get_cpl)(vcpu) != 0 ||
+	if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) ||
+	    !kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE))
+		return kvm_handle_invalid_op(vcpu);
+
+	if ((is_protmode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) != 0) ||
 	    __kvm_set_xcr(vcpu, kvm_rcx_read(vcpu), kvm_read_edx_eax(vcpu))) {
 		kvm_inject_gp(vcpu, 0);
 		return 1;