Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

From: Peter Gonda
Date: Wed Oct 02 2024 - 10:59:59 EST

On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@xxxxxxx> wrote:
>
> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>
> 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
> 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.
>
> Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this
> new module parameter has to added to the CCP driver.
>
> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> ---
> arch/x86/kvm/svm/sev.c | 26 ++++++++++++++----
> drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++
> include/linux/psp-sev.h | 12 +++++++--
> 3 files changed, 83 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0b851ef937f2..a345b4111ad6 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev)
> misc_cg_uncharge(type, sev->misc_cg, 1);
> }
>
> -static int sev_asid_new(struct kvm_sev_info *sev)
> +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type)
> {
> /*
> * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid.
> @@ -199,6 +199,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 && sev->es_active) {
> + if (vm_type == KVM_X86_SNP_VM)
> + max_asid = snp_max_snp_asid;
> + else
> + min_asid = snp_max_snp_asid + 1;
> + }
> again:
> asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid);
> if (asid > max_asid) {
> @@ -440,7 +452,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> if (vm_type == KVM_X86_SNP_VM)
> sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
>
> - ret = sev_asid_new(sev);
> + ret = sev_asid_new(sev, vm_type);
> if (ret)
> goto e_no_asid;
>
> @@ -3059,14 +3071,18 @@ 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 (snp_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 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 :
> + 0, min_sev_asid - 1);
> + }
> 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, snp_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 564daf748293..77900abb1b46 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 cipher_text_hiding = true;
> +module_param(cipher_text_hiding, bool, 0444);
> +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding");
> +
> +static int max_snp_asid;
> +module_param(max_snp_asid, int, 0444);
> +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding");

My read of the spec is if Ciphertext hiding is not enabled there is no
additional split in the ASID space. Am I understanding that correctly?
If so, I don't think we want to enable ciphertext hiding by default
because it might break whatever management of ASIDs systems already
have. For instance right now we have to split SEV-ES and SEV ASIDS,
and SNP guests need SEV-ES ASIDS. This change would half the # of SNP
enable ASIDs on a system.

Also should we move the ASID splitting code to be all in one place?
Right now KVM handles it in sev_hardware_setup().

> +
> 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;
> +EXPORT_SYMBOL(snp_cipher_text_hiding);
> +
> +/* MAX_SNP_ASID */
> +unsigned int snp_max_snp_asid;
> +EXPORT_SYMBOL(snp_max_snp_asid);
> +
> static bool psp_dead;
> static int psp_timeout;
>
> @@ -1064,6 +1080,38 @@ 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 & SNP_CIPHER_TEXT_HIDING_SUPPORTED) &&
> + sev->snp_plat_status.ciphertext_hiding_cap) {
> + /* Retrieve SEV CPUID information */
> + edx = cpuid_edx(0x8000001f);
> + /* Do sanity checks on user-defined MAX_SNP_ASID */
> + if (max_snp_asid >= edx) {
> + dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n",
> + edx - 1);
> + max_snp_asid = edx - 1;
> + }
> + snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2;
> +
> + snp_cipher_text_hiding = 1;
> + data->ciphertext_hiding_en = 1;
> + data->max_snp_asid = snp_max_snp_asid;
> +
> + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");
> + }
> +}
> +
> static void snp_get_platform_data(void)
> {
> struct sev_device *sev = psp_master->sev_data;
> @@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error)
> }
>
> memset(&data, 0, sizeof(data));
> +
> + if (cipher_text_hiding)
> + 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 6068a89839e1..2102248bd436 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;
> +extern unsigned int snp_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;
>
> /**
> @@ -841,6 +847,8 @@ struct snp_feature_info {
> u32 edx;
> } __packed;
>
> +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED BIT(3)
> +
> #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>
> /**
> --
> 2.34.1
>
>