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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `vmcb02_insn_bytes_fresh` flag unconditionally marks DecodeAssist instruction bytes as fresh for all reflected hardware VM-exits, which incorrectly leaks stale VMCB02 bytes to L1 if multiple L0 exits occur during a single nested run.
--

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

KVM: nSVM: Track hardware-provided instruction bytes

This commit clears the VMCB02 instruction-byte fields before each nested run
and tracks whether a hardware VM-Exit being reflected to L1 includes fresh
instruction bytes. This tracking is intended to distinguish bytes produced by
the current hardware VM-Exit from stale VMCB02 state.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 73f37b050d0a0..d08c30e9a6f44 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c

[ ... ]

> @@ -868,7 +876,10 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>  	/*
>  	 * Filled at exit: exit_code, exit_info_1, exit_info_2, exit_int_info,
>  	 * exit_int_info_err, next_rip, insn_len, insn_bytes.
> +	 * Clear stale DecodeAssist data before L2 runs.
>  	 */
> +	nested_svm_clear_insn_bytes(vmcb02);
> +	svm->nested.vmcb02_insn_bytes_fresh = false;

[Severity: High]
Does this only clear the instruction bytes during the initial L1 to L2 VMRUN?

If L0 handles an intermediate exit (like a nested page fault) and resumes L2
without re-clearing vmcb02, could the instruction bytes from that intermediate
exit be left behind?

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c7c1f1527c3cf..b85e43112db9e 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 set the flag to true for all #PF exits that are
pending for L1?

Hardware doesn't provide DecodeAssist for instruction fetch faults. If an L2
guest triggers a data NPF followed immediately by a fetch fault, could this
leak stale instruction bytes from the data NPF to L1?

> +
> +	return r;
>  }

[ ... ]

> @@ -3723,7 +3729,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]
Does unconditionally passing true here ignore whether the hardware actually
provided DecodeAssist for this exit?

If the exit didn't populate DecodeAssist, could this pass stale bytes from a
previous exit to L1?

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