Re: [PATCH] KVM: SVM: Clear VMCB save area instead of entire VMCB on shutdown intercept

[Shivansh Dhiman <shivansh.dhiman@amd.com>]

On 06-10-26 10:32, Sean Christopherson wrote:
> On Tue, Aug 25, 2026, Shivansh Dhiman wrote:
> 
>> On 24-08-26 19:21, sashiko-bot@kernel.org wrote:
>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>> --- a/arch/x86/kvm/svm/svm.c
>>>> +++ b/arch/x86/kvm/svm/svm.c
>>> [ ... ]
>>>> @@ -2164,18 +2164,23 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
>>> [ ... ]
>>>>  	if (!is_sev_es_guest(vcpu)) {
>>>> -		clear_page(svm->vmcb);
>>>> +		struct vmcb_control_area *control = &svm->vmcb->control;
>>>> +
>>>> +		memset(&svm->vmcb->save, 0, sizeof(svm->vmcb->save));
>>>> +		control->int_ctl &= ~GENMASK(15, 0);
>>>> +		control->int_state = 0;
>>>
>>> [Severity: High]
>>> Since clear_page() was replaced with targeted clearing, do we also need to
>>> clear control->event_inj and control->event_inj_err here?
>>>
>>> According to the AMD APM, hardware does not clear the EVENTINJ field in the
>>> VMCB on a VM exit, which leaves the VALID bit set if an exit occurs during
>>> event delivery (such as a triple fault leading to SVM_EXIT_SHUTDOWN).
>>>
>>> If the EVENTINJ field is left intact across this shutdown reset, will KVM
>>> execute the subsequent VMRUN with the stale EVENTINJ_VALID flag still set?
>>>
>>> Could this cause KVM to spuriously inject the old fault into the newly
>>> reset real-mode vCPU on its first instruction, leading to an immediate
>>> crash or an infinite triple-fault loop?
>> The latest APM says otherwise. The #VMEXIT sequence in APM ends with "clear
>> EVENTINJ field in VMCB". Hardware clears the field on every exit, so there
>> is nothing stale to carry across the shutdown reset and the scenario can't
>> arise.
> 
> But that seemingly unconditional statement has hidden clauses[*]:
> 
>  : >> The second half isn't. Clearing EVENTINJ is not part of injection, it's part of
>  : >> #VMEXIT, and it's unconditional.
>  : > So that doesn't mesh with the above comment, which says:
>  : >
>  : >   Hardware clears EVENTINJ field when it injects an event.
>  : >
>  : > And it begs the question of how this patch is at all useful.  Because all this
>  : > fancy new paranoia is clearly generating #VMEXITs, and if #VMEXIT unconditionally
>  : > clears control->event_inj, I don't see how control->event_inj can be non-zero if
>  : > KVM attempted VMRUN.
>  : >
>  : > I.e. either this is all broken, or the APM is buggy.
>  : 
>  : My wording in the comment is misleading. The clear is part of the #VMEXIT path
>  : out of guest mode, where it is indeed unconditional. However, VMRUN can
>  : terminate even before the guest mode is ever entered. With ESMTP, the hardware
>  : can give out a garden variety of exits at the sync point. In this case we
>  : get a VMEXIT that implies the VMRUN never actually ran any guest code then
>  : EVENTINJ won't be cleared. Since the exit code isn't a reliable discriminator,
>  : a non-zero EVENTINJ is the only way to tell.
> 
> Ignoring that IMO the APM needs to be updated to clarify exactly when EVENTINJ
> is cleared and when it isn't, I don't see any point in not manually clearing the
> field on SHUTDOWN.  Common sense would say that it's unnecessary as SHUTDOWN can
> only occur if VMRUN gets into the guest, but the cost is completely neglible and
> there is zero chance EVENTINJ *needs* to be retained, unlike say the PML index.
> 
> We can certainly add a comment saying it's paranoid, but I do think we should
> manually clear EVENTINJ and friend, e.g.

Agreed. I'll clear event_inj and event_inj_err with your comment and post v2
soon.

Thanks,
Shivansh

> 
> 	/*
> 	 * EVENTINJ is cleared on #VMEXIT, but only if VMRUN fully entered the
> 	 * guest.  Manually clear the fields even though it should be redundant,
> 	 * as there is no downside to doing so.
> 	 */
> 	control->event_inj = 0;
> 	control->event_inj_err = 0;
> 
> [*] https://lore.kernel.org/all/d946de080e3dd34844a665c167179e4910fdc9d4.1789399214.git.prsampat@amd.com