| 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
| ||
|
Message-ID: <afafee48-4a74-85a0-8455-640eb2d59948@amd.com>
Date: Thu, 15 Aug 2024 10:58:14 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Ashish Kalra <Ashish.Kalra@....com>, seanjc@...gle.com,
pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, hpa@...or.com, herbert@...dor.apana.org.au
Cc: x86@...nel.org, john.allen@....com, davem@...emloft.net,
michael.roth@....com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-crypto@...r.kernel.org
Subject: Re: [PATCH 3/3] x86/sev: Add SEV-SNP CipherTextHiding support
On 8/12/24 14:42, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@....com>
>
> Ciphertext hiding prevents host accesses from reading the ciphertext of
> SNP guest private memory. Instead of reading ciphertext, the host reads
> will see constant default values (0xff).
>
> Ciphertext hiding separates the ASID space into SNP guest ASIDs and host
> ASIDs. All SNP active guests must have an ASID less than or equal to
> MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests
> (SEV and SEV-ES) must be greater than MAX_SNP_ASID.
>
> This patch-set adds a new module parameter to the CCP driver defined as
> psp_max_snp_asid which is a user configurable MAX_SNP_ASID to define the
> system-wide maximum SNP ASID value. If this value is not set, then the
> ASID space is equally divided between SEV-SNP and SEV-ES guests.
Add something here to talk about how cipher text hiding needs to be
enabled on SNP_INIT_EX and so the module parameters are part of the CCP
driver.
Not sure if having KVM invoke the CCP, at KVM module load time, to
perform the SNP_INIT_EX would make sense or not. That would allow all
the cipher text hiding support parameters to be in KVM. Lets see what
others think.
>
> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> ---
> arch/x86/kvm/svm/sev.c | 24 ++++++++++++++---
> drivers/crypto/ccp/sev-dev.c | 50 ++++++++++++++++++++++++++++++++++++
> include/linux/psp-sev.h | 10 ++++++--
> 3 files changed, 79 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 532df12b43c5..954ef99a1aa8 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -173,6 +173,9 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
>
> static int sev_asid_new(struct kvm_sev_info *sev)
> {
> + struct kvm_svm *svm = container_of(sev, struct kvm_svm, sev_info);
> + struct kvm *kvm = &svm->kvm;
Why not just add the vm-type to the input parameters?
> +
> /*
> * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
> * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1.
> @@ -199,6 +202,18 @@ static int sev_asid_new(struct kvm_sev_info *sev)
>
> mutex_lock(&sev_bitmap_lock);
>
> + /*
> + * When CipherTextHiding is enabled, all SNP guests must have an
> + * ASID less than or equal to MAX_SNP_ASID provided on the
> + * SNP_INIT_EX command and all the SEV-ES guests must have
> + * an ASID greater than MAX_SNP_ASID.
> + */
> + if (snp_cipher_text_hiding_en && sev->es_active) {
> + if (kvm->arch.vm_type == KVM_X86_SNP_VM)
> + max_asid = max_snp_asid;
> + else
> + min_asid = max_snp_asid + 1;
> + }
> again:
> asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
> if (asid > max_asid) {
> @@ -3058,14 +3073,17 @@ void __init sev_hardware_setup(void)
> "unusable" :
> "disabled",
> min_sev_asid, max_sev_asid);
> - if (boot_cpu_has(X86_FEATURE_SEV_ES))
> + if (boot_cpu_has(X86_FEATURE_SEV_ES)) {
> + if (max_snp_asid >= (min_sev_asid - 1))
> + sev_es_supported = false;
> pr_info("SEV-ES %s (ASIDs %u - %u)\n",
> sev_es_supported ? "enabled" : "disabled",
> - min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> + min_sev_asid > 1 ? max_snp_asid ? max_snp_asid + 1 : 1 : 0, min_sev_asid - 1);
Maybe put the last parameter on a separate line to make this a little
easier to read?
> + }
> if (boot_cpu_has(X86_FEATURE_SEV_SNP))
> pr_info("SEV-SNP %s (ASIDs %u - %u)\n",
> sev_snp_supported ? "enabled" : "disabled",
> - min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1);
> + min_sev_asid > 1 ? 1 : 0, max_snp_asid ? : min_sev_asid - 1);
>
> sev_enabled = sev_supported;
> sev_es_enabled = sev_es_supported;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index eefb481db5af..9ee81a6defc5 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
> module_param(psp_init_on_probe, bool, 0444);
> MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>
> +static bool psp_cth_enabled = true;
This is a static, so how about just calling this cipher_text_hiding.
> +module_param(psp_cth_enabled, bool, 0444);
> +MODULE_PARM_DESC(psp_cth_enabled, " if true, the PSP will enable Cipher Text Hiding");
> +
> +static int psp_max_snp_asid;
Also a static, so just call this max_snp_asid.
> +module_param(psp_max_snp_asid, int, 0444);
> +MODULE_PARM_DESC(psp_max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding");
> +
> MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
> MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
> MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
> MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
>
> +/* Cipher Text Hiding Enabled */
> +bool snp_cipher_text_hiding_en;
> +EXPORT_SYMBOL(snp_cipher_text_hiding_en);
I think you drop the "_en" and just have snp_cipher_text_hiding.
> +
> +/* MAX_SNP_ASID */
> +unsigned int max_snp_asid;
> +EXPORT_SYMBOL(max_snp_asid);
This should have "snp_" to provide better namespace isolation.
> +
> static bool psp_dead;
> static int psp_timeout;
>
> @@ -1053,6 +1069,36 @@ static void snp_set_hsave_pa(void *arg)
> wrmsrl(MSR_VM_HSAVE_PA, 0);
> }
>
> +static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error)
> +{
> + struct psp_device *psp = psp_master;
> + struct sev_device *sev;
> + unsigned int edx;
> +
> + sev = psp->sev_data;
> +
> + /*
> + * Check if CipherTextHiding feature is supported and enabled
> + * in the Platform/BIOS.
> + */
> + if (sev->feat_info.ecx & FEAT_CIPHERTEXTHIDING_SUPPORTED &&
> + sev->snp_plat_status.ciphertext_hiding_cap) {
I'm not sure you need both checks. Either the platform status or the
feature info check should be enough. Can you check on that?
> + /* Retrieve SEV CPUID information */
> + edx = cpuid_edx(0x8000001f);
> + /* Do sanity checks on user-defined MAX_SNP_ASID */
> + if (psp_max_snp_asid > (edx - 1)) {
How about:
if (psp_max_snp_asid >= edx) {
> + dev_info(sev->dev, "user-defined MAX_SNP_ASID invalid, limiting to %d\n",
max_snp_asid module parameter is not valid, limiting to %d\n
> + edx - 1);
> + psp_max_snp_asid = edx - 1;
> + }
> + max_snp_asid = psp_max_snp_asid ? : (edx - 1) / 2;
Add a blank line here
> + snp_cipher_text_hiding_en = 1;
> + data->ciphertext_hiding_en = 1;
> + data->max_snp_asid = max_snp_asid;
Ditto
Thanks,
Tom
> + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");
> + }
> +}
> +
> static void sev_cache_snp_platform_status_and_discover_features(void)
> {
> struct sev_device *sev = psp_master->sev_data;
> @@ -1181,6 +1227,10 @@ static int __sev_snp_init_locked(int *error)
> }
>
> memset(&data, 0, sizeof(data));
> +
> + if (psp_cth_enabled)
> + sev_snp_enable_ciphertext_hiding(&data, error);
> +
> data.init_rmp = 1;
> data.list_paddr_en = 1;
> data.list_paddr = __psp_pa(snp_range_list);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index d46d73911a76..a26b43c2eab9 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -27,6 +27,9 @@ enum sev_state {
> SEV_STATE_MAX
> };
>
> +extern bool snp_cipher_text_hiding_en;
> +extern unsigned int max_snp_asid;
> +
> /**
> * SEV platform and guest management commands
> */
> @@ -746,10 +749,13 @@ struct sev_data_snp_guest_request {
> struct sev_data_snp_init_ex {
> u32 init_rmp:1;
> u32 list_paddr_en:1;
> - u32 rsvd:30;
> + u32 rapl_dis:1;
> + u32 ciphertext_hiding_en:1;
> + u32 rsvd:28;
> u32 rsvd1;
> u64 list_paddr;
> - u8 rsvd2[48];
> + u16 max_snp_asid;
> + u8 rsvd2[46];
> } __packed;
>
> /**
Powered by blists - more mailing lists