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

[Tom Lendacky]

On 9/17/24 15:16, Ashish Kalra 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;
> +	}

Add a blank line here.

>  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);

I think this might be easier to read if you align everything based on
the conditions, e.g.:

		pr_info("SEV-ES %s (ASIDs %u - %u)\n",
			sev_es_supported ? "enabled" : "disabled",
			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");

I agree with Peter that this should be false by default to maintain
backward compatibility.

> +
> +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");

MODULE_PARM_DESC(max_snp_asid, "  the maximum SNP ASID available when Cipher Text Hiding is enabled");

> +
>  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) {

Remove the indentation here by just doing:

	if (!(sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED))
		return;

	if (!sev->snp_plat_status.ciphertext_hiding_cap)
		return;

> +		/* Retrieve SEV CPUID information */

Remove this comment and ...

> +		edx = cpuid_edx(0x8000001f);
> +		/* Do sanity checks on user-defined MAX_SNP_ASID */

... move this comment above the cpuid_edx() call.

> +		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;

s/1/true/

> +		data->ciphertext_hiding_en = 1;
> +		data->max_snp_asid = snp_max_snp_asid;
> +
> +		dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n");

"SEV-SNP cipher text hiding enabled"

Thanks,
Tom

> +	}
> +}
> +
>  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
>  
>  /**