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

On 7/30/2026 8:31 PM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

Returning zero is intentional. If the starting EIP is already beyond
the CS limit, no instruction byte is architecturally fetchable from
CS:EIP, and the APM specifies an instruction length of zero in that
case.

A matching emulator fetch cache should not normally contain bytes for
this condition. The emulator fetch path uses __linearize() with
X86EMUL_F_FETCH and checks the same CS limit before populating its cache.

Also, a 16-bit code segment does not imply that EIP should be
unconditionally truncated to 16 bits here. KVM's emulator checks the
full non-64-bit EIP against the segment limit, which is consistent with
this code.

Thanks,
Tina

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