linux-kernel - Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <249694b0-2afd-f653-a443-124e510f4a4c@amd.com>
Date:   Wed, 6 Sep 2023 15:19:47 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Peter Gonda <pgonda@...gle.com>, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Joerg Roedel <joro@...tes.org>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more
 metadata

On 9/6/23 15:11, Sean Christopherson wrote:
> On Wed, Sep 06, 2023, Tom Lendacky wrote:
>> On 9/6/23 10:14, Peter Gonda wrote:
>>> Currently if an SEV-ES VM shuts down userspace sees KVM_RUN struct with
>>
>> s/down userspace/down, userspace/
> 
> Heh, yeah, I read that the same way you did.
> 
>>> only the INVALID_ARGUMENT. This is a very limited amount of information
>>> to debug the situation. Instead KVM can return a
>>> KVM_EXIT_SHUTDOWN to alert userspace the VM is shutting down and
>>> is not usable any further.
>>>
>>> Signed-off-by: Peter Gonda <pgonda@...gle.com>
>>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>>> Cc: Sean Christopherson <seanjc@...gle.com>
>>> Cc: Tom Lendacky <thomas.lendacky@....com>
>>> Cc: Joerg Roedel <joro@...tes.org>
>>> Cc: Borislav Petkov <bp@...en8.de>
>>> Cc: x86@...nel.org
>>> Cc: kvm@...r.kernel.org
>>> Cc: linux-kernel@...r.kernel.org
>>>
>>> ---
>>>    arch/x86/kvm/svm/svm.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 956726d867aa..cecf6a528c9b 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
>>>    	 * The VM save area has already been encrypted so it
>>>    	 * cannot be reinitialized - just terminate.
>>>    	 */
>>> -	if (sev_es_guest(vcpu->kvm))
>>> -		return -EINVAL;
>>> +	if (sev_es_guest(vcpu->kvm)) {
>>> +		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
>>> +		return 0;
>>> +	}
>>
>> Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also set
>> at the end of the function and I don't think kvm_vcpu_reset() clears the
>> value from kvm_run, you could just set kvm_run->exit_reason on entry and
>> just return 0 early for an SEV-ES guest.
> 
> kvm_run is writable by userspace though, so KVM can't rely on kvm_run->exit_reason
> for correctness.
> 
> And IIUC, the VMSA is also toast, i.e. doing anything other than marking the VM
> dead is futile, no?

I was just saying that "kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;" is in 
the shutdown_interception() function twice now (at both exit points of the 
function) and can probably just be moved to the top of the function and be 
common for both exit points, now, right?

I'm not saying to get rid of it, just set it sooner.

Thanks,
Tom