linux-kernel - Re: [PATCH RFC v7 37/64] KVM: SVM: Add KVM_SNP

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: <fd23ee51-ec47-717d-7cce-1d79db8b6bd3@amd.com>
Date:   Thu, 5 Jan 2023 17:37:20 -0600
From:   "Kalra, Ashish" <ashish.kalra@....com>
To:     Jarkko Sakkinen <jarkko@...nel.org>,
        Michael Roth <michael.roth@....com>
Cc:     kvm@...r.kernel.org, linux-coco@...ts.linux.dev,
        linux-mm@...ck.org, linux-crypto@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
        jroedel@...e.de, thomas.lendacky@....com, hpa@...or.com,
        ardb@...nel.org, pbonzini@...hat.com, seanjc@...gle.com,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        luto@...nel.org, dave.hansen@...ux.intel.com, slp@...hat.com,
        pgonda@...gle.com, peterz@...radead.org,
        srinivas.pandruvada@...ux.intel.com, rientjes@...gle.com,
        dovmurik@...ux.ibm.com, tobin@....com, bp@...en8.de,
        vbabka@...e.cz, kirill@...temov.name, ak@...ux.intel.com,
        tony.luck@...el.com, marcorr@...gle.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, alpergun@...gle.com,
        dgilbert@...hat.com, harald@...fian.com,
        Brijesh Singh <brijesh.singh@....com>,
        Pavan Kumar Paluri <papaluri@....com>
Subject: Re: [PATCH RFC v7 37/64] KVM: SVM: Add KVM_SNP_INIT command

Hello Jarkko,

On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote:
> On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
>>   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   {
>>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   		return ret;
>>   
>>   	sev->active = true;
>> -	sev->es_active = argp->id == KVM_SEV_ES_INIT;
>> +	sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>> +	sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
>>   	asid = sev_asid_new(sev);
>>   	if (asid < 0)
>>   		goto e_no_asid;
>>   	sev->asid = asid;
>>   
>> -	ret = sev_platform_init(&argp->error);
>> +	if (sev->snp_active) {
>> +		ret = verify_snp_init_flags(kvm, argp);
>> +		if (ret)
>> +			goto e_free;
>> +
>> +		ret = sev_snp_init(&argp->error, false);
>> +	} else {
>> +		ret = sev_platform_init(&argp->error);
>> +	}
> 
> Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
> in order?
> 
> Since there is a hardware constraint that SNP init needs to always happen
> before platform init, shouldn't SNP init happen as part of
> __sev_platform_init_locked() instead?
> 

On Genoa there is currently an issue that if we do an SNP_INIT before an 
SEV_INIT and then attempt to launch a SEV guest that may fail, so we 
need to keep SNP INIT and SEV INIT separate.

We need to provide a way to run (existing) SEV guests on a system that 
supports SNP without doing an SNP_INIT at all.

This is done using psp_init_on_probe parameter of the CCP module to 
avoid doing either SNP/SEV firmware initialization during module load 
and then defer the firmware initialization till someone launches a guest 
of one flavor or the other.

And then sev_guest_init() does either SNP or SEV firmware init depending 
on the type of the guest being launched.

> I found these call sites for __sev_platform_init_locked(), none of which
> follow the correct call order:
> 
> * sev_guest_init()

As explained above, this call site is important for deferring the 
firmware initialization to an actual guest launch.

> * sev_ioctl_do_pek_csr
> * sev_ioctl_do_pdh_export()
> * sev_ioctl_do_pek_import()
> * sev_ioctl_do_pek_pdh_gen()
> * sev_pci_init()
> 
> For me it looks like a bit flakky API use to have sev_snp_init() as an API
> call.
> 
> I would suggest to make SNP init internal to the ccp driver and take care
> of the correct orchestration over there.
>

Due to Genoa issue, we may still need SNP init and SEV init to be 
invoked separately outside the CCP driver.

> Also, how it currently works in this patch set, if the firmware did not
> load correctly, SNP init halts the whole system. The version check needs
> to be in all call paths.
> 

Yes, i agree with that.

Thanks,
Ashish