Re: [PATCH v3 7/9] KVM: nSVM: Fetch missing DecodeAssist bytes for synthesized #NPF/#PF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Already-prepared instruction bytes are discarded if the instruction pointer exceeds the CS limit (or due to residual upper bits in 16-bit mode).
--

--- Patch [7]: [PATCH v3 7/9] KVM: nSVM: Fetch missing DecodeAssist bytes for synthesized #NPF/#PF ---
commit 96d13188ab4298d9414e0345853cc8950d39abb9
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: nSVM: Fetch missing DecodeAssist bytes for synthesized #NPF/#PF

This commit limits the preparation of synthesized state to matching the
emulator fetch cache and fetches any missing tail bytes through L2 guest
page tables. If the emulator cache is empty, it attempts to fetch the full
window from the L2 RIP, avoiding fetches for SEV guests.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index cfb686c29bb47..cf2b6bb8126ff 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c

[ ... ]

> +static u8 nested_svm_fetch_insn_bytes(struct kvm_vcpu *vcpu, u8 *bytes,
> +				      u8 count, u8 max_bytes)
> +{
> +	struct kvm_pagewalk *gva_walk = &vcpu->arch.gva_walk;
> +	u64 access = PFERR_FETCH_MASK;
> +	gva_t rip = kvm_get_linear_rip(vcpu);
> +	struct x86_exception e;
> +
> +	if (kvm_x86_call(get_cpl)(vcpu) == 3)
> +		access |= PFERR_USER_MASK;
> +
> +	if (!is_64_bit_mode(vcpu)) {
> +		u32 eip = kvm_rip_read(vcpu);
> +		u32 limit = to_svm(vcpu)->vmcb->save.cs.limit;
> +
> +		if (eip > limit)
> +			return 0;

[Severity: Medium]
Should this return count instead of 0?

If an L2 guest in 16-bit or 32-bit mode causes a nested page fault where
eip > limit evaluates to true (for example, in 16-bit mode if kvm_rip_read()
retains residual upper 16-bits), returning 0 here causes the caller to
overwrite vmcb12->control.insn_len with 0. 

This would inadvertently discard any already-prepared valid instruction bytes
that were passed in via the count parameter, forcing the L1 hypervisor to
manually read the instruction from L2 memory and defeating the DecodeAssist
optimization.

> +		max_bytes = min_t(u64, max_bytes, (u64)limit - eip + 1);
> +	}
> +
> +	count = min(count, max_bytes);
> +
> +	while (count < max_bytes) {

[ ... ]

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