Re: [PATCH v2 4/8] KVM: nSVM: Propagate hardware DecodeAssist bytes to VMCB12

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware DecodeAssist bytes are not propagated to VMCB12 for intercepted #PF exits when KVM reflects the page fault, breaking the feature.
- [Medium] KVM reads control fields directly from guest-controlled memory (VMCB12) to determine if DecodeAssist bytes should be copied, creating a TOCTOU race condition.
--

--- Patch [4]: [PATCH v2 4/8] KVM: nSVM: Propagate hardware DecodeAssist bytes to VMCB12 ---
commit 3aed32a695a3f569675b1941e25ebc814c2dc09d
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: nSVM: Propagate hardware DecodeAssist bytes to VMCB12

When DecodeAssists is exposed to L1, copy fresh hardware instruction
bytes from VMCB02 to VMCB12 for data #NPF and intercepted #PF VM-Exits.
Clear VMCB12's instruction-byte state for instruction-fetch faults,
unrelated exits, and exits without fresh VMCB02 state so that stale bytes
are not exposed to L1.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 70e2ee3af78b2..9e12eab7eed97 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
[ ... ]
> +static bool nested_svm_vmexit_supports_insn_bytes(struct kvm_vcpu *vcpu,
> +						  const struct vmcb *vmcb12)
> +{
> +	u64 exit_code = vmcb12->control.exit_code;
> +
> +	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_DECODEASSISTS))
> +		return false;
> +
> +	if (exit_code != SVM_EXIT_NPF &&
> +	    exit_code != SVM_EXIT_EXCP_BASE + PF_VECTOR)
> +		return false;
> +
> +	return !(vmcb12->control.exit_info_1 & PFERR_FETCH_MASK);

[Severity: Medium]
Could this create a TOCTOU race condition by reading control fields directly
from guest-controlled memory? 

Because vmcb12 is mapped to guest physical memory, a malicious L1 guest on a
sibling vCPU could concurrently modify these fields. For instance, changing
the exit code to SVM_EXIT_NPF after KVM has already populated it.

Would this trick KVM into bypassing the intended security clearing logic,
thereby leaking DecodeAssist bytes for unrelated exits that were supposed to
be scrubbed? Should KVM read these fields from the trusted host-managed
vmcb02 instead?

> +}
> +
> +/*
> + * Rebuild VMCB12's DecodeAssist bytes for the nested VM-Exit.  Use fresh
> + * hardware VMCB02 state when available; otherwise report a zero byte count.
> + */
> +static void nested_svm_update_vmcb12_insn_bytes(struct kvm_vcpu *vcpu,
> +						struct vmcb *vmcb12,
> +						struct vmcb *vmcb02)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	bool vmcb02_insn_bytes_fresh = svm->nested.vmcb02_insn_bytes_fresh;
> +
> +	svm->nested.vmcb02_insn_bytes_fresh = false;
> +
> +	nested_svm_clear_insn_bytes(vmcb12);
> +
> +	if (!nested_svm_vmexit_supports_insn_bytes(vcpu, vmcb12))
> +		return;
> +
> +	if (vmcb02_insn_bytes_fresh)
> +		nested_svm_copy_insn_bytes(vmcb12, vmcb02);

[Severity: High]
Does KVM fail to propagate hardware DecodeAssist bytes for intercepted #PF
exits when it reflects the page fault?

When L0 intercepts a #PF and reflects it to L1 via
nested_svm_inject_exception_vmexit():

nested_svm_inject_exception_vmexit() {
    ...
    vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + ex->vector;
    ...
    nested_svm_vmexit(svm);
}

It seems nested_svm_inject_exception_vmexit() fails to set the
vmcb02_insn_bytes_fresh flag. Because the flag remains false, the valid
hardware instruction bytes are silently dropped here instead of being copied
to VMCB12. 

Will this cause L1 hypervisors relying on DecodeAssist for #PF to receive an
empty instruction buffer and fail to decode the faulting instruction?

> +}

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