Re: [PATCH v5 25/26] KVM: nSVM: Sanitize control fields copied from VMCB12

[Sean Christopherson]

On Fri, Feb 06, 2026, Yosry Ahmed wrote:
> Make sure all fields used from VMCB12 in creating the VMCB02 are
> sanitized, such that no unhandled or reserved bits end up in the VMCB02.
> 
> The following control fields are read from VMCB12 and have bits that are
> either reserved or not handled/advertised by KVM: tlb_ctl, int_ctl,
> int_state, int_vector, event_inj, misc_ctl, and misc_ctl2.
> 
> The following fields do not require any extra sanitizing:
> - int_ctl: bits from VMCB12 are copied bit-by-bit as needed.
> - misc_ctl: only used in consistency checks (particularly NP_ENABLE).
> - misc_ctl2: bits from VMCB12 are copied bit-by-bit as needed.
> 
> For the remaining fields, make sure only defined bits are copied from
> L1's VMCB12 into KVM'cache by defining appropriate masks where needed.
> The only exception is tlb_ctl, which is unused, so remove it.
> 
> Opportunistically cleanup ignoring the lower bits of {io/msr}pm_base_pa
> in __nested_copy_vmcb_control_to_cache() by using PAGE_MASK. Also, move
> the ASID copying ahead with other special cases, and expand the comment
> about the ASID being copied only for consistency checks.

Stop. Bundling. Changes.

This is not a hypothetical situation, bundling small changes like this is quite
literally making review take 3-4x longer than it should.

The interrupt changes are trivial to review.

The I/O and MSR bitmap changes are also easy enough, but I wanted to double that
PAGE_MASK does indeed equal ~0x0fffULL (__PHYSICAL_MASK is the one that can be dynamic).

I disagree the the tlb_ctl change.

Moving the ASID handling is _completely_ superfluous.

Combining any two of those is annoying to deal with.  Combining all of them wastes
a non-trivial amount of time.  What should have taken me ~5 minutes to review is
dragging into 20+ minutes, because I keep having to cross-reference the changelog
with the code to understand WTF is going on.

Just stop doing it, please.
 
> Suggested-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Signed-off-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> ---
>  arch/x86/include/asm/svm.h |  5 +++++
>  arch/x86/kvm/svm/nested.c  | 28 +++++++++++++++-------------
>  arch/x86/kvm/svm/svm.h     |  1 -
>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index c169256c415f..fe3b6d9cea31 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -222,6 +222,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define X2APIC_MODE_SHIFT 30
>  #define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
>  
> +#define SVM_INT_VECTOR_MASK GENMASK(7, 0)
> +
>  #define SVM_INTERRUPT_SHADOW_MASK	BIT_ULL(0)
>  #define SVM_GUEST_INTERRUPT_MASK	BIT_ULL(1)
>  
> @@ -635,6 +637,9 @@ static inline void __unused_size_checks(void)
>  #define SVM_EVTINJ_VALID (1 << 31)
>  #define SVM_EVTINJ_VALID_ERR (1 << 11)
>  
> +#define SVM_EVTINJ_RESERVED_BITS ~(SVM_EVTINJ_VEC_MASK | SVM_EVTINJ_TYPE_MASK | \
> +				   SVM_EVTINJ_VALID_ERR | SVM_EVTINJ_VALID)
> +
>  #define SVM_EXITINTINFO_VEC_MASK SVM_EVTINJ_VEC_MASK
>  #define SVM_EXITINTINFO_TYPE_MASK SVM_EVTINJ_TYPE_MASK
>  
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 0a7bb01f5404..c87738962970 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -499,32 +499,35 @@ void __nested_copy_vmcb_control_to_cache(struct kvm_vcpu *vcpu,
>  	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_NPT))
>  		to->misc_ctl &= ~SVM_MISC_ENABLE_NP;
>  
> -	to->iopm_base_pa        = from->iopm_base_pa;
> -	to->msrpm_base_pa       = from->msrpm_base_pa;
> +	/*
> +	 * Copy the ASID here because nested_vmcb_check_controls() will check
> +	 * it.  The ASID could be invalid, or conflict with another VM's ASID ,

Spurious space before the command.

> +	 * so it should never be used directly to run L2.
> +	 */
> +	to->asid = from->asid;
> +
> +	/* Lower bits of IOPM_BASE_PA and MSRPM_BASE_PA are ignored */
> +	to->iopm_base_pa        = from->iopm_base_pa & PAGE_MASK;
> +	to->msrpm_base_pa       = from->msrpm_base_pa & PAGE_MASK;
>>  	to->tsc_offset          = from->tsc_offset;
> -	to->tlb_ctl             = from->tlb_ctl;

I don't think we should completely drop tlb_ctl.  KVM doesn't do anything with
vmcb12's tlb_ctl only because we haven't addressed the TODO list in
nested_svm_transition_tlb_flush().  I think I would rather update this code to
sanitize the field now, as opposed to waiting until we address that TODO.

KVM advertises X86_FEATURE_FLUSHBYASID, so I think we can do the right thing
without having to speculate on what the future will bring.

Alternatively, we could add a TODO here or update the one in
nested_svm_transition_tlb_flush(), but that seems like more overall work than
just hardening the code.