Re: [PATCH v3 3/9] KVM: nSVM: Track hardware-provided instruction bytes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch clears instruction bytes only on nested VMRUN, causing stale DecodeAssist data from L0-handled exits to be falsely flagged as fresh during subsequent intercepts.
- [High] Hardware-provided DecodeAssist data and the fresh flag are lost across live migration for a pending nested #PF.
--

--- Patch [3]: [PATCH v3 3/9] KVM: nSVM: Track hardware-provided instruction bytes ---
commit e944fdb4c1650228b5e210eab6c8b6fe2761112a
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: nSVM: Track hardware-provided instruction bytes
    
DecodeAssists provides instruction bytes for nested page faults and
intercepted page faults caused by data accesses.  When reflecting such an
exit to L1, KVM needs to distinguish bytes produced by the current hardware
VM-Exit from stale VMCB02 state.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b63234a2feb8d..6e2ceb700567c 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c

[ ... ]

> @@ -1953,14 +1953,20 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
>  static int pf_interception(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> -
>  	u64 fault_address = svm->vmcb->control.exit_info_2;
>  	u64 error_code = svm->vmcb->control.exit_info_1;
> +	int r;
> +
> +	r = kvm_handle_page_fault(vcpu, error_code, fault_address,
> +				  static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> +				  svm->vmcb->control.insn_bytes : NULL,
> +				  svm->vmcb->control.insn_len);
>  
> -	return kvm_handle_page_fault(vcpu, error_code, fault_address,
> -			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> -			svm->vmcb->control.insn_bytes : NULL,
> -			svm->vmcb->control.insn_len);
> +	if (is_guest_mode(vcpu) && vcpu->arch.exception_vmexit.pending &&
> +	    vcpu->arch.exception_vmexit.vector == PF_VECTOR)
> +		svm->nested.vmcb02_insn_bytes_fresh = true;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this unconditionally mark the instruction bytes as fresh for any PF_VECTOR,
including instruction fetches where DecodeAssist is inactive?

If so, could this result in stale bytes from a previous hardware VM-Exit
being incorrectly synthesized to the L1 hypervisor?

> +
> +	return r;
>  }

[ ... ]

> @@ -3725,7 +3731,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  		vmexit = nested_svm_exit_special(svm);
>  
>  		if (vmexit == NESTED_EXIT_CONTINUE)
> -			vmexit = nested_svm_exit_handled(svm);
> +			vmexit = nested_svm_exit_handled(svm, true);
                                                              ^^^^

[Severity: High]
By passing true here for all NESTED_EXIT_CONTINUE VM-exits, does this
incorrectly flag stale bytes as fresh for exits where DecodeAssist is
not active?

For example, if L2 causes a data #PF handled by L0 (which writes insn_bytes)
and is resumed without clearing the instruction bytes, and then L2 causes a
different exit intercepted by L1 (like an MSR intercept), will this pass the
stale instruction bytes to L1 as fresh?

>  
>  		if (vmexit == NESTED_EXIT_DONE)
>  			return 1;

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index a327bf751ecd5..96d2471a03c8c 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -242,6 +242,12 @@ struct svm_nested_state {
>  	 * on its side.
>  	 */
>  	bool force_msr_bitmap_recalc;
> +
> +	/*
> +	 * True if VMCB02 contains DecodeAssist instruction bytes from the
> +	 * hardware VM-Exit currently being reflected to L1.
> +	 */
> +	bool vmcb02_insn_bytes_fresh;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Will this tracking state be lost across live migration?

Since it appears this flag is added to the nested state structure but omitted
from KVM_GET_NESTED_STATE serialization, if a migration occurs while a nested
exit is pending, won't the target host fail to receive the instruction bytes
and potentially cause emulation failure in L1?

>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785411877.git.zhang_wei@open-hieco.net?part=3