linux-kernel - Re: [PATCH RFC v7 25/64] crypto: ccp: Add support to initialize the AMD-SP for SEV-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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5bb741b8-8daa-028d-d920-826e1396dd87@amd.com>
Date:   Thu, 5 Jan 2023 16:40:29 -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>
Subject: Re: [PATCH RFC v7 25/64] crypto: ccp: Add support to initialize the
 AMD-SP for SEV-SNP

Hello Jarkko,

On 12/31/2022 9:32 AM, Jarkko Sakkinen wrote:
> On Wed, Dec 14, 2022 at 01:40:17PM -0600, Michael Roth wrote:
>> From: Brijesh Singh <brijesh.singh@....com>
>>
>> Before SNP VMs can be launched, the platform must be appropriately
>> configured and initialized. Platform initialization is accomplished via
>> the SNP_INIT command. Make sure to do a WBINVD and issue DF_FLUSH
>> command to prepare for the first SNP guest launch after INIT.
>>
>> During the execution of SNP_INIT command, the firmware configures
>> and enables SNP security policy enforcement in many system components.
>> Some system components write to regions of memory reserved by early
>> x86 firmware (e.g. UEFI). Other system components write to regions
>> provided by the operation system, hypervisor, or x86 firmware.
>> Such system components can only write to HV-fixed pages or Default
>> pages. They will error when attempting to write to other page states
>> after SNP_INIT enables their SNP enforcement.
>>
>> Starting in SNP firmware v1.52, the SNP_INIT_EX command takes a list of
>> system physical address ranges to convert into the HV-fixed page states
>> during the RMP initialization. If INIT_RMP is 1, hypervisors should
>> provide all system physical address ranges that the hypervisor will
>> never assign to a guest until the next RMP re-initialization.
>> For instance, the memory that UEFI reserves should be included in the
>> range list. This allows system components that occasionally write to
>> memory (e.g. logging to UEFI reserved regions) to not fail due to
>> RMP initialization and SNP enablement.
>>
>> Co-developed-by: Ashish Kalra <ashish.kalra@....com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> Signed-off-by: Michael Roth <michael.roth@....com>
>> ---
>>   drivers/crypto/ccp/sev-dev.c | 225 +++++++++++++++++++++++++++++++++++
>>   drivers/crypto/ccp/sev-dev.h |   2 +
>>   include/linux/psp-sev.h      |  17 +++
>>   3 files changed, 244 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 9d84720a41d7..af20420bd6c2 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/fs_struct.h>
>>   
>>   #include <asm/smp.h>
>> +#include <asm/e820/types.h>
>>   
>>   #include "psp-dev.h"
>>   #include "sev-dev.h"
>> @@ -34,6 +35,10 @@
>>   #define SEV_FW_FILE		"amd/sev.fw"
>>   #define SEV_FW_NAME_SIZE	64
>>   
>> +/* Minimum firmware version required for the SEV-SNP support */
>> +#define SNP_MIN_API_MAJOR	1
>> +#define SNP_MIN_API_MINOR	51
>> +
>>   static DEFINE_MUTEX(sev_cmd_mutex);
>>   static struct sev_misc_dev *misc_dev;
>>   
>> @@ -76,6 +81,13 @@ static void *sev_es_tmr;
>>   #define NV_LENGTH (32 * 1024)
>>   static void *sev_init_ex_buffer;
>>   
>> +/*
>> + * SEV_DATA_RANGE_LIST:
>> + *   Array containing range of pages that firmware transitions to HV-fixed
>> + *   page state.
>> + */
>> +struct sev_data_range_list *snp_range_list;
>> +
>>   static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>>   {
>>   	struct sev_device *sev = psp_master->sev_data;
>> @@ -830,6 +842,186 @@ static int sev_update_firmware(struct device *dev)
>>   	return ret;
>>   }
>>   
>> +static void snp_set_hsave_pa(void *arg)
>> +{
>> +	wrmsrl(MSR_VM_HSAVE_PA, 0);
>> +}
>> +
>> +static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>> +{
>> +	struct sev_data_range_list *range_list = arg;
>> +	struct sev_data_range *range = &range_list->ranges[range_list->num_elements];
>> +	size_t size;
>> +
>> +	if ((range_list->num_elements * sizeof(struct sev_data_range) +
>> +	     sizeof(struct sev_data_range_list)) > PAGE_SIZE)
>> +		return -E2BIG;
>> +
>> +	switch (rs->desc) {
>> +	case E820_TYPE_RESERVED:
>> +	case E820_TYPE_PMEM:
>> +	case E820_TYPE_ACPI:
>> +		range->base = rs->start & PAGE_MASK;
>> +		size = (rs->end + 1) - rs->start;
>> +		range->page_count = size >> PAGE_SHIFT;
>> +		range_list->num_elements++;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __sev_snp_init_locked(int *error)
>> +{
>> +	struct psp_device *psp = psp_master;
>> +	struct sev_data_snp_init_ex data;
>> +	struct sev_device *sev;
>> +	int rc = 0;
>> +
>> +	if (!psp || !psp->sev_data)
>> +		return -ENODEV;
>> +
>> +	sev = psp->sev_data;
>> +
>> +	if (sev->snp_initialized)
>> +		return 0;
> 
> Shouldn't this follow this check:
> 
>          if (sev->state == SEV_STATE_INIT) {
>                  /* debug printk about possible incorrect call order */
>                  return -ENODEV;
>          }
> 
> It is game over for SNP, if SEV_CMD_INIT{_EX} got first, which means that
> this should not proceed.


But, how will SEV_CMD_INIT_EX happen before as sev_pci_init() which is 
invoked during CCP module load/initialization, will first try to do 
sev_snp_init() if SNP is supported, before it invokes 
sev_platform_init() to do SEV firmware initialization ?

Thanks,
Ashish