Re: [PATCH v4 5/9] KVM: x86: Track emulator-originated nested #PF VM-Exits

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added `has_emulator_context` field is left uninitialized or improperly cleared when queueing non-nested exceptions and restoring state from userspace.
- [Low] The `has_emulator_context` field is populated but never read anywhere in the codebase.
--

--- Patch [5]: [PATCH v4 5/9] KVM: x86: Track emulator-originated nested #PF VM-Exits ---
commit 224b6e38736ada4f6f56c322ba1950bffe0ab244
Author: Tina Zhang <[email protected]>

KVM: x86: Track emulator-originated nested #PF VM-Exits

Add has_emulator_context to kvm_queued_exception to record whether a
queued exception VM-Exit originated from the current emulator exception.

> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 283847619ff88..eb9f1b9afeb7d 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -688,6 +688,7 @@ struct kvm_queued_exception {
>  	bool pending;
>  	bool injected;
>  	bool has_error_code;
> +	bool has_emulator_context;

[Severity: Low]
Does this code ever read the newly added has_emulator_context field? It
looks like it is populated here and throughout the exception queueing paths,
but is never actually consumed.

>  	u8 vector;
>  	u32 error_code;
>  	unsigned long payload;

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d94b59140c452..75d00340c71fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -473,7 +476,8 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, unsigned int nr,
>  	if (is_guest_mode(vcpu) &&
>  	    kvm_nested_call(is_exception_vmexit)(vcpu, nr, error_code)) {
>  		kvm_queue_exception_vmexit(vcpu, nr, has_error, error_code,
> -					   has_payload, payload);
> +					   has_payload, payload,
> +					   has_emulator_context);
>  		return;
>  	}

[Severity: Medium]
Is it possible to leave the has_emulator_context field uninitialized here?

Looking at the rest of kvm_multiple_exception(), the new has_emulator_context
parameter is passed in, but it isn't assigned to the vcpu exception state in
the normal queue path:

    if (!vcpu->arch.exception.pending && !vcpu->arch.exception.injected) {
    queue:
        vcpu->arch.exception.pending = true;
        ...
        vcpu->arch.exception.has_payload = has_payload;
        vcpu->arch.exception.payload = payload;
        return;
    }

Could this result in leaking stale state for has_emulator_context?

Furthermore, does this flag also need to be explicitly cleared in
kvm_vcpu_ioctl_x86_set_vcpu_events() when restoring exception state from
userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5