| Message ID | 20240517173926.965351-13-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
On 5/18/2024 1:38 AM, Sean Christopherson wrote: > Reject KVM_CAP_X86_DISABLE_EXITS if userspace attempts to disable MWAIT or > HLT exits and KVM previously reported (via KVM_CHECK_EXTENSION) that > disabling the exit(s) is not allowed. E.g. because MWAIT isn't supported > or the CPU doesn't have an aways-running APIC timer, or because KVM is aways-running -> always-running > configured to mitigate cross-thread vulnerabilities. > > Cc: Kechen Lu <kechenl@nvidia.com> > Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts") > Fixes: 6f0f2d5ef895 ("KVM: x86: Mitigate the cross-thread return address predictions bug") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 54 ++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4cb0c150a2f8..c729227c6501 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4590,6 +4590,20 @@ static inline bool kvm_can_mwait_in_guest(void) > boot_cpu_has(X86_FEATURE_ARAT); > } > > +static u64 kvm_get_allowed_disable_exits(void) > +{ > + u64 r = KVM_X86_DISABLE_EXITS_PAUSE; > + > + if (!mitigate_smt_rsb) { > + r |= KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_CSTATE; > + > + if (kvm_can_mwait_in_guest()) > + r |= KVM_X86_DISABLE_EXITS_MWAIT; > + } > + return r; > +} > + > #ifdef CONFIG_KVM_HYPERV > static int kvm_ioctl_get_supported_hv_cpuid(struct kvm_vcpu *vcpu, > struct kvm_cpuid2 __user *cpuid_arg) > @@ -4726,15 +4740,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > r = KVM_CLOCK_VALID_FLAGS; > break; > case KVM_CAP_X86_DISABLE_EXITS: > - r = KVM_X86_DISABLE_EXITS_PAUSE; > - > - if (!mitigate_smt_rsb) { > - r |= KVM_X86_DISABLE_EXITS_HLT | > - KVM_X86_DISABLE_EXITS_CSTATE; > - > - if (kvm_can_mwait_in_guest()) > - r |= KVM_X86_DISABLE_EXITS_MWAIT; > - } > + r |= kvm_get_allowed_disable_exits(); Nit: Just use "=". > break; > case KVM_CAP_X86_SMM: > if (!IS_ENABLED(CONFIG_KVM_SMM)) > @@ -6565,33 +6571,29 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > break; > case KVM_CAP_X86_DISABLE_EXITS: > r = -EINVAL; > - if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) > + if (cap->args[0] & ~kvm_get_allowed_disable_exits()) > break; > > mutex_lock(&kvm->lock); > if (kvm->created_vcpus) > goto disable_exits_unlock; > > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > - kvm->arch.pause_in_guest = true; > - > #define SMT_RSB_MSG "This processor is affected by the Cross-Thread Return Predictions vulnerability. " \ > "KVM_CAP_X86_DISABLE_EXITS should only be used with SMT disabled or trusted guests." > > - if (!mitigate_smt_rsb) { > - if (boot_cpu_has_bug(X86_BUG_SMT_RSB) && cpu_smt_possible() && > - (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) > - pr_warn_once(SMT_RSB_MSG); > - > - if ((cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) && > - kvm_can_mwait_in_guest()) > - kvm->arch.mwait_in_guest = true; > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) > - kvm->arch.hlt_in_guest = true; > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) > - kvm->arch.cstate_in_guest = true; > - } > + if (!mitigate_smt_rsb && boot_cpu_has_bug(X86_BUG_SMT_RSB) && > + cpu_smt_possible() && > + (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) > + pr_warn_once(SMT_RSB_MSG); > > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > + kvm->arch.pause_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) > + kvm->arch.mwait_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) > + kvm->arch.hlt_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) > + kvm->arch.cstate_in_guest = true; > r = 0; > disable_exits_unlock: > mutex_unlock(&kvm->lock);
On Wed, May 22, 2024, Binbin Wu wrote: > On 5/18/2024 1:38 AM, Sean Christopherson wrote: > > @@ -4726,15 +4740,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > > r = KVM_CLOCK_VALID_FLAGS; > > break; > > case KVM_CAP_X86_DISABLE_EXITS: > > - r = KVM_X86_DISABLE_EXITS_PAUSE; > > - > > - if (!mitigate_smt_rsb) { > > - r |= KVM_X86_DISABLE_EXITS_HLT | > > - KVM_X86_DISABLE_EXITS_CSTATE; > > - > > - if (kvm_can_mwait_in_guest()) > > - r |= KVM_X86_DISABLE_EXITS_MWAIT; > > - } > > + r |= kvm_get_allowed_disable_exits(); > > Nit: Just use "=". Yowsers, that's more than a nit, that's downright bad code, it just happens to be functionally ok. Thanks again for the reviews!
On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > Reject KVM_CAP_X86_DISABLE_EXITS if userspace attempts to disable MWAIT or > HLT exits and KVM previously reported (via KVM_CHECK_EXTENSION) that > disabling the exit(s) is not allowed. E.g. because MWAIT isn't supported > or the CPU doesn't have an aways-running APIC timer, or because KVM is > configured to mitigate cross-thread vulnerabilities. > > Cc: Kechen Lu <kechenl@nvidia.com> > Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts") > Fixes: 6f0f2d5ef895 ("KVM: x86: Mitigate the cross-thread return address predictions bug") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 54 ++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4cb0c150a2f8..c729227c6501 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4590,6 +4590,20 @@ static inline bool kvm_can_mwait_in_guest(void) > boot_cpu_has(X86_FEATURE_ARAT); > } > > +static u64 kvm_get_allowed_disable_exits(void) > +{ > + u64 r = KVM_X86_DISABLE_EXITS_PAUSE; > + > + if (!mitigate_smt_rsb) { > + r |= KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_CSTATE; > + > + if (kvm_can_mwait_in_guest()) > + r |= KVM_X86_DISABLE_EXITS_MWAIT; > + } > + return r; > +} > + > #ifdef CONFIG_KVM_HYPERV > static int kvm_ioctl_get_supported_hv_cpuid(struct kvm_vcpu *vcpu, > struct kvm_cpuid2 __user *cpuid_arg) > @@ -4726,15 +4740,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > r = KVM_CLOCK_VALID_FLAGS; > break; > case KVM_CAP_X86_DISABLE_EXITS: > - r = KVM_X86_DISABLE_EXITS_PAUSE; > - > - if (!mitigate_smt_rsb) { > - r |= KVM_X86_DISABLE_EXITS_HLT | > - KVM_X86_DISABLE_EXITS_CSTATE; > - > - if (kvm_can_mwait_in_guest()) > - r |= KVM_X86_DISABLE_EXITS_MWAIT; > - } > + r |= kvm_get_allowed_disable_exits(); > break; > case KVM_CAP_X86_SMM: > if (!IS_ENABLED(CONFIG_KVM_SMM)) > @@ -6565,33 +6571,29 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > break; > case KVM_CAP_X86_DISABLE_EXITS: > r = -EINVAL; > - if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) > + if (cap->args[0] & ~kvm_get_allowed_disable_exits()) > break; > > mutex_lock(&kvm->lock); > if (kvm->created_vcpus) > goto disable_exits_unlock; > > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > - kvm->arch.pause_in_guest = true; > - > #define SMT_RSB_MSG "This processor is affected by the Cross-Thread Return Predictions vulnerability. " \ > "KVM_CAP_X86_DISABLE_EXITS should only be used with SMT disabled or trusted guests." > > - if (!mitigate_smt_rsb) { > - if (boot_cpu_has_bug(X86_BUG_SMT_RSB) && cpu_smt_possible() && > - (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) > - pr_warn_once(SMT_RSB_MSG); > - > - if ((cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) && > - kvm_can_mwait_in_guest()) > - kvm->arch.mwait_in_guest = true; > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) > - kvm->arch.hlt_in_guest = true; > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) > - kvm->arch.cstate_in_guest = true; > - } > + if (!mitigate_smt_rsb && boot_cpu_has_bug(X86_BUG_SMT_RSB) && > + cpu_smt_possible() && > + (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) > + pr_warn_once(SMT_RSB_MSG); > > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > + kvm->arch.pause_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) > + kvm->arch.mwait_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) > + kvm->arch.hlt_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) > + kvm->arch.cstate_in_guest = true; > r = 0; > disable_exits_unlock: > mutex_unlock(&kvm->lock); Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On 5/18/2024 1:38 AM, Sean Christopherson wrote: > Reject KVM_CAP_X86_DISABLE_EXITS if userspace attempts to disable MWAIT or > HLT exits and KVM previously reported (via KVM_CHECK_EXTENSION) that > disabling the exit(s) is not allowed. E.g. because MWAIT isn't supported > or the CPU doesn't have an aways-running APIC timer, or because KVM is > configured to mitigate cross-thread vulnerabilities. > > Cc: Kechen Lu <kechenl@nvidia.com> > Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts") > Fixes: 6f0f2d5ef895 ("KVM: x86: Mitigate the cross-thread return address predictions bug") > Signed-off-by: Sean Christopherson <seanjc@google.com> Just realize the same issue when reading the MWAIT code then find your this fix. Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/kvm/x86.c | 54 ++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4cb0c150a2f8..c729227c6501 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4590,6 +4590,20 @@ static inline bool kvm_can_mwait_in_guest(void) > boot_cpu_has(X86_FEATURE_ARAT); > } > > +static u64 kvm_get_allowed_disable_exits(void) > +{ > + u64 r = KVM_X86_DISABLE_EXITS_PAUSE; > + > + if (!mitigate_smt_rsb) { > + r |= KVM_X86_DISABLE_EXITS_HLT | > + KVM_X86_DISABLE_EXITS_CSTATE; > + > + if (kvm_can_mwait_in_guest()) > + r |= KVM_X86_DISABLE_EXITS_MWAIT; > + } > + return r; > +} > + > #ifdef CONFIG_KVM_HYPERV > static int kvm_ioctl_get_supported_hv_cpuid(struct kvm_vcpu *vcpu, > struct kvm_cpuid2 __user *cpuid_arg) > @@ -4726,15 +4740,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > r = KVM_CLOCK_VALID_FLAGS; > break; > case KVM_CAP_X86_DISABLE_EXITS: > - r = KVM_X86_DISABLE_EXITS_PAUSE; > - > - if (!mitigate_smt_rsb) { > - r |= KVM_X86_DISABLE_EXITS_HLT | > - KVM_X86_DISABLE_EXITS_CSTATE; > - > - if (kvm_can_mwait_in_guest()) > - r |= KVM_X86_DISABLE_EXITS_MWAIT; > - } > + r |= kvm_get_allowed_disable_exits(); > break; > case KVM_CAP_X86_SMM: > if (!IS_ENABLED(CONFIG_KVM_SMM)) > @@ -6565,33 +6571,29 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > break; > case KVM_CAP_X86_DISABLE_EXITS: > r = -EINVAL; > - if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) > + if (cap->args[0] & ~kvm_get_allowed_disable_exits()) sigh. KVM_X86_DISABLE_VALID_EXITS has no user now. But we cannot remove it since it's in uapi header, right? > break; > > mutex_lock(&kvm->lock); > if (kvm->created_vcpus) > goto disable_exits_unlock; > > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > - kvm->arch.pause_in_guest = true; > - > #define SMT_RSB_MSG "This processor is affected by the Cross-Thread Return Predictions vulnerability. " \ > "KVM_CAP_X86_DISABLE_EXITS should only be used with SMT disabled or trusted guests." > > - if (!mitigate_smt_rsb) { > - if (boot_cpu_has_bug(X86_BUG_SMT_RSB) && cpu_smt_possible() && > - (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) > - pr_warn_once(SMT_RSB_MSG); > - > - if ((cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) && > - kvm_can_mwait_in_guest()) > - kvm->arch.mwait_in_guest = true; > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) > - kvm->arch.hlt_in_guest = true; > - if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) > - kvm->arch.cstate_in_guest = true; > - } > + if (!mitigate_smt_rsb && boot_cpu_has_bug(X86_BUG_SMT_RSB) && > + cpu_smt_possible() && > + (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) > + pr_warn_once(SMT_RSB_MSG); > > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > + kvm->arch.pause_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) > + kvm->arch.mwait_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) > + kvm->arch.hlt_in_guest = true; > + if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) > + kvm->arch.cstate_in_guest = true; > r = 0; > disable_exits_unlock: > mutex_unlock(&kvm->lock);
On Fri, Jul 12, 2024, Xiaoyao Li wrote: > On 5/18/2024 1:38 AM, Sean Christopherson wrote: > > @@ -6565,33 +6571,29 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > > break; > > case KVM_CAP_X86_DISABLE_EXITS: > > r = -EINVAL; > > - if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) > > + if (cap->args[0] & ~kvm_get_allowed_disable_exits()) > > sigh. > > KVM_X86_DISABLE_VALID_EXITS has no user now. But we cannot remove it since > it's in uapi header, right? We can, actually. Forcing userspace to make changes when userspace updates their copy of the headers is ok (building directly against kernel headers is discouraged).
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4cb0c150a2f8..c729227c6501 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4590,6 +4590,20 @@ static inline bool kvm_can_mwait_in_guest(void) boot_cpu_has(X86_FEATURE_ARAT); } +static u64 kvm_get_allowed_disable_exits(void) +{ + u64 r = KVM_X86_DISABLE_EXITS_PAUSE; + + if (!mitigate_smt_rsb) { + r |= KVM_X86_DISABLE_EXITS_HLT | + KVM_X86_DISABLE_EXITS_CSTATE; + + if (kvm_can_mwait_in_guest()) + r |= KVM_X86_DISABLE_EXITS_MWAIT; + } + return r; +} + #ifdef CONFIG_KVM_HYPERV static int kvm_ioctl_get_supported_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 __user *cpuid_arg) @@ -4726,15 +4740,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) r = KVM_CLOCK_VALID_FLAGS; break; case KVM_CAP_X86_DISABLE_EXITS: - r = KVM_X86_DISABLE_EXITS_PAUSE; - - if (!mitigate_smt_rsb) { - r |= KVM_X86_DISABLE_EXITS_HLT | - KVM_X86_DISABLE_EXITS_CSTATE; - - if (kvm_can_mwait_in_guest()) - r |= KVM_X86_DISABLE_EXITS_MWAIT; - } + r |= kvm_get_allowed_disable_exits(); break; case KVM_CAP_X86_SMM: if (!IS_ENABLED(CONFIG_KVM_SMM)) @@ -6565,33 +6571,29 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, break; case KVM_CAP_X86_DISABLE_EXITS: r = -EINVAL; - if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) + if (cap->args[0] & ~kvm_get_allowed_disable_exits()) break; mutex_lock(&kvm->lock); if (kvm->created_vcpus) goto disable_exits_unlock; - if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) - kvm->arch.pause_in_guest = true; - #define SMT_RSB_MSG "This processor is affected by the Cross-Thread Return Predictions vulnerability. " \ "KVM_CAP_X86_DISABLE_EXITS should only be used with SMT disabled or trusted guests." - if (!mitigate_smt_rsb) { - if (boot_cpu_has_bug(X86_BUG_SMT_RSB) && cpu_smt_possible() && - (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) - pr_warn_once(SMT_RSB_MSG); - - if ((cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) && - kvm_can_mwait_in_guest()) - kvm->arch.mwait_in_guest = true; - if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) - kvm->arch.hlt_in_guest = true; - if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) - kvm->arch.cstate_in_guest = true; - } + if (!mitigate_smt_rsb && boot_cpu_has_bug(X86_BUG_SMT_RSB) && + cpu_smt_possible() && + (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE)) + pr_warn_once(SMT_RSB_MSG); + if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) + kvm->arch.pause_in_guest = true; + if (cap->args[0] & KVM_X86_DISABLE_EXITS_MWAIT) + kvm->arch.mwait_in_guest = true; + if (cap->args[0] & KVM_X86_DISABLE_EXITS_HLT) + kvm->arch.hlt_in_guest = true; + if (cap->args[0] & KVM_X86_DISABLE_EXITS_CSTATE) + kvm->arch.cstate_in_guest = true; r = 0; disable_exits_unlock: mutex_unlock(&kvm->lock);
Reject KVM_CAP_X86_DISABLE_EXITS if userspace attempts to disable MWAIT or HLT exits and KVM previously reported (via KVM_CHECK_EXTENSION) that disabling the exit(s) is not allowed. E.g. because MWAIT isn't supported or the CPU doesn't have an aways-running APIC timer, or because KVM is configured to mitigate cross-thread vulnerabilities. Cc: Kechen Lu <kechenl@nvidia.com> Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts") Fixes: 6f0f2d5ef895 ("KVM: x86: Mitigate the cross-thread return address predictions bug") Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 54 ++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 26 deletions(-)