Re: [PATCH v2 5/8] KVM: nSVM: Use emulator bytes for synthesized nested #NPF/#PF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Flawed heuristic in `nested_svm_prepare_synthesized_insn_bytes` leaks stale instruction bytes to L1 DecodeAssist fields during native #PF/#NPF exits.
--

--- Patch [5]: [PATCH v2 5/8] KVM: nSVM: Use emulator bytes for synthesized nested #NPF/#PF ---
commit d6e2023803dd9b80aeaf2560525bc97569fd2d75
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: nSVM: Use emulator bytes for synthesized nested #NPF/#PF

This commit updates KVM to synthesize DecodeAssist instruction bytes from the
emulator fetch cache for KVM-generated #NPF/#PF exits. It introduces a
heuristic to match the emulator context against the current instruction pointer
to populate the buffer, which is then copied to VMCB12 when constructing the
nested VM-Exit.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 9e12eab7eed97..8cee9aeaf4f27 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -69,26 +69,72 @@ static bool nested_svm_vmexit_supports_insn_bytes(struct kvm_vcpu *vcpu,

[ ... ]

> +static void nested_svm_prepare_synthesized_insn_bytes(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct nested_svm_insn_bytes *insn_bytes =
> +		&svm->nested.synthesized_insn_bytes;
> +	const u8 max_bytes = sizeof(insn_bytes->insn_bytes);
> +	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
> +
> +	nested_svm_clear_synthesized_insn_bytes(svm);
> +
> +	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_DECODEASSISTS))
> +		return;
> +
> +	if (!ctxt || ctxt->eip != kvm_rip_read(vcpu) ||

[Severity: High]
Does this heuristic incorrectly validate the emulator cache based solely on RIP
equality?

If an instruction is successfully emulated (such as an MMIO access), the
emulator advances ctxt->eip to the next instruction's RIP. If the guest then
resumes natively and that next instruction triggers an intercepted #PF,
ctxt->eip will exactly match kvm_rip_read(vcpu).

Since ctxt->fetch.data still contains the uncleared bytes of the previously
emulated instruction, won't this cause KVM to incorrectly synthesize and
inject those stale instruction bytes into L1's VMCB12 DecodeAssist fields?

> +	    ctxt->fetch.end < ctxt->fetch.data ||
> +	    ctxt->fetch.end > ctxt->fetch.data + max_bytes)
> +		return;
> +
> +	insn_bytes->insn_len = ctxt->fetch.end - ctxt->fetch.data;
> +	memcpy(insn_bytes->insn_bytes, ctxt->fetch.data,
> +	       insn_bytes->insn_len);
> +	insn_bytes->prepared = true;
> +}

[ ... ]

> @@ -1709,6 +1758,9 @@ static void nested_svm_inject_exception_vmexit(struct kvm_vcpu *vcpu)
>  			vmcb->control.exit_info_2 = ex->payload;
>  		else
>  			vmcb->control.exit_info_2 = vcpu->arch.cr2;
> +
> +		if (!ex->has_error_code || !(ex->error_code & PFERR_FETCH_MASK))
> +			nested_svm_prepare_synthesized_insn_bytes(vcpu);

[Severity: High]
Is it safe to call the synthesis function here for #PF exits without knowing
if the queued exception actually originated from the emulator?

Calling this unconditionally allows hardware-generated page faults to trigger
the heuristic above. An unprivileged L2 guest could potentially exploit this
data leak by placing a faulting instruction immediately after an MMIO
instruction to spoof DecodeAssist bytes provided to L1.

>  	} else if (ex->vector == DB_VECTOR) {
>  		/* See kvm_check_and_inject_events().  */
>  		kvm_deliver_exception_payload(vcpu, ex);

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