On Tue, Dec 09, 2025, Yosry Ahmed wrote:
On Tue, Dec 09, 2025 at 10:42:21AM -0800, Sean Christopherson wrote:
On Tue, Dec 09, 2025, Yosry Ahmed wrote:
On Tue, Dec 09, 2025 at 10:26:31AM -0800, Sean Christopherson wrote:
On Tue, Dec 09, 2025, Yosry Ahmed wrote:
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h > index f6fb70ddf7272..3e805a43ffcdb 100644 > --- a/arch/x86/kvm/svm/svm.h > +++ b/arch/x86/kvm/svm/svm.h > @@ -552,7 +552,8 @@ static inline bool gif_set(struct vcpu_svm *svm) > > static inline bool nested_npt_enabled(struct vcpu_svm *svm) > { > - return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE; > + return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_NPT) && > + svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
I would rather rely on Kevin's patch to clear unsupported features.
Not sure how Kevin's patch is relevant here, could you please clarify?
Doh, Kevin's patch only touches intercepts. What I was trying to say is that I would rather sanitize the snapshot (the approach Kevin's patch takes with the intercepts), as opposed to guarding the accessor. That way we can't have bugs where KVM checks svm->nested.ctl.nested_ctl directly and bypasses the caps check.
I see, so clear SVM_NESTED_CTL_NP_ENABLE in __nested_copy_vmcb_control_to_cache() instead.
If I drop the guest_cpu_cap_has() check here I will want to leave a comment so that it's obvious to readers that SVM_NESTED_CTL_NP_ENABLE is sanitized elsewhere if the guest cannot use NPTs. Alternatively, I can just keep the guest_cpu_cap_has() check as documentation and a second line of defense.
Any preferences?
Honestly, do nothing. I want to solidify sanitizing the cache as standard behavior, at which point adding a comment implies that nested_npt_enabled() is somehow special, i.e. that it _doesn't_ follow the standard.
Does this apply to patch 12 as well? In that patch I int_vector,
I <something>?
int_state, and event_inj when copying them to VMCB02 in nested_vmcb02_prepare_control(). Mainly because nested_vmcb02_prepare_control() already kinda filters what to copy from VMCB12 (e.g. int_ctl), so it seemed like a better fit.
Do I keep that as-is, or do you prefer that I also sanitize these fields when copying to the cache in nested_copy_vmcb_control_to_cache()?
I don't think I follow. What would the sanitization look like? Note, I don't think we need to completely sanitize _every_ field. The key fields are ones where KVM consumes and/or acts on the field.