Skip to contentSkip to conversation

Add SEV-SNP CipherTextHiding feature support

PATCH Superseded Ashish Kalra 2 participants 8 messages Aug 12, 2024 – Aug 15, 2024

Subsystems (6)
x86 KVM Crypto API KVM x86 +2 more subsystems

v1 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Ashish Kalra

author maintainer
#0 lore raw

cover Add SEV-SNP CipherTextHiding feature support

From: Ashish Kalra <[email protected]>

Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory. Instead of reading ciphertext, the host 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 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.

Ashish Kalra (3):
crypto: ccp: Extend SNP_PLATFORM_STATUS command
crypto: ccp: Add support for SNP_FEATURE_INFO command x86/sev: Add SEV-SNP CipherTextHiding support

 arch/x86/kvm/svm/sev.c       | 24 ++++++++--
 drivers/crypto/ccp/sev-dev.c | 90 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 ++
 include/linux/psp-sev.h      | 41 +++++++++++++++-
 include/uapi/linux/psp-sev.h | 10 +++-
 5 files changed, 162 insertions(+), 6 deletions(-)
Signature · 1 line
2.34.1
author maintainer
#1 lore raw

PATCH 1/3 crypto: ccp: Extend SNP_PLATFORM_STATUS command

From: Ashish Kalra <[email protected]>

Extend information returned about SNP platform's status and capabilities such as SNP_FEATURE_INFO command availability, ciphertext hiding enabled and ciphertext hiding capability.

Signed-off-by Ashish Kalra <[email protected]>
---
 include/uapi/linux/psp-sev.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
include/uapi/linux/psp-sev.h +9 -1
index 2289b7c76c59..19a0a284b798 100644
@@ -177,6 +177,10 @@ struct sev_user_data_get_id2 {
  * @mask_chip_id: whether chip id is present in attestation reports or not
  * @mask_chip_key: whether attestation reports are signed or not
  * @vlek_en: VLEK (Version Loaded Endorsement Key) hashstick is loaded
+ * @feature_info: whether SNP_FEATURE_INFO command is available
+ * @rapl_dis: whether RAPL is disabled
+ * @ciphertext_hiding_cap: whether platform has ciphertext hiding enabled
+ * @ciphertext_hiding_en: whether ciphertext hiding is enabled
  * @rsvd1: reserved
  * @guest_count: the number of guest currently managed by the firmware
  * @current_tcb_version: current TCB version

@@ -192,7 +196,11 @@ struct sev_user_data_snp_status {
 	__u32 mask_chip_id:1;		/* Out */
 	__u32 mask_chip_key:1;		/* Out */
 	__u32 vlek_en:1;		/* Out */
-	__u32 rsvd1:29;
+	__u32 feature_info:1;		/* Out */
+	__u32 rapl_dis:1;		/* Out */
+	__u32 ciphertext_hiding_cap:1;	/* Out */
+	__u32 ciphertext_hiding_en:1;	/* Out */
+	__u32 rsvd1:25;
 	__u32 guest_count;		/* Out */
 	__u64 current_tcb_version;	/* Out */
 	__u64 reported_tcb_version;	/* Out */
Signature · 1 line
2.34.1
maintainer
#2 lore raw

Re: [PATCH 1/3] crypto: ccp: Extend SNP_PLATFORM_STATUS command

Quoted 5 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Extend information returned about SNP platform's status and capabilities such as SNP_FEATURE_INFO command availability, ciphertext hiding enabled and ciphertext hiding capability.

Just saying something like:

Define new bit-field definitions returned by the SNP_PLATFORM_STATUS command.

You're not actually extending the command or struct.

Quoted 11 lines
Signed-off-by Ashish Kalra <[email protected]>
---
 include/uapi/linux/psp-sev.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
include/uapi/linux/psp-sev.h +0 -0
index 2289b7c76c59..19a0a284b798 100644

include/uapi/linux/psp-sev.h struct sev_user_data_get_id2 { · line 182

Quoted 6 lines
include/uapi/linux/psp-sev.h +3 -0
@@ -177,6 +177,10 @@ struct sev_user_data_get_id2 {
  * @mask_chip_id: whether chip id is present in attestation reports or not
  * @mask_chip_key: whether attestation reports are signed or not
  * @vlek_en: VLEK (Version Loaded Endorsement Key) hashstick is loaded
+ * @feature_info: whether SNP_FEATURE_INFO command is available
+ * @rapl_dis: whether RAPL is disabled
+ * @ciphertext_hiding_cap: whether platform has ciphertext hiding enabled

s/enabled/capability/

Thanks,
Tom

Quoted 17 lines
include/uapi/linux/psp-sev.h +6 -1
+ * @ciphertext_hiding_en: whether ciphertext hiding is enabled
  * @rsvd1: reserved
  * @guest_count: the number of guest currently managed by the firmware
  * @current_tcb_version: current TCB version

@@ -192,7 +196,11 @@ struct sev_user_data_snp_status {
 	__u32 mask_chip_id:1;		/* Out */
 	__u32 mask_chip_key:1;		/* Out */
 	__u32 vlek_en:1;		/* Out */
-	__u32 rsvd1:29;
+	__u32 feature_info:1;		/* Out */
+	__u32 rapl_dis:1;		/* Out */
+	__u32 ciphertext_hiding_cap:1;	/* Out */
+	__u32 ciphertext_hiding_en:1;	/* Out */
+	__u32 rsvd1:25;
 	__u32 guest_count;		/* Out */
 	__u64 current_tcb_version;	/* Out */
 	__u64 reported_tcb_version;	/* Out */
author maintainer
#3 lore raw

PATCH 2/3 crypto: ccp: Add support for SNP_FEATURE_INFO command

In reply to Ashish Kalra

From: Ashish Kalra <[email protected]>

The FEATURE_INFO command provides host and guests a programmatic means to learn about the supported features of the currently loaded firmware. FEATURE_INFO command leverages the same mechanism as the CPUID instruction. Instead of using the CPUID instruction to retrieve Fn8000_0024, software can use FEATURE_INFO.

During CCP module initialization, after firmware update, the SNP platform status and feature information from CPUID 0x8000_0024, sub-function 0, are cached in the sev_device structure.

Signed-off-by Ashish Kalra <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 40 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 +++
 include/linux/psp-sev.h      | 31 ++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)
drivers/crypto/ccp/sev-dev.c +40 -0
index 9810edbb272d..eefb481db5af 100644
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_feature_info);
 	default:				return 0;
 	}
 

@@ -1052,6 +1053,43 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void sev_cache_snp_platform_status_and_discover_features(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_snp_feature_info snp_feat_info;
+	struct sev_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;

@@ -2395,6 +2433,8 @@ void sev_pci_init(void)
 	if (sev_update_firmware(sev->dev) == 0)
 		sev_get_api_version();
 
+	sev_cache_snp_platform_status_and_discover_features();
+
 	/* Initialize the platform */
 	args.probe = true;
 	rc = sev_platform_init(&args);
drivers/crypto/ccp/sev-dev.h +3 -0
index 3e4e5574e88a..11e571e87e18 100644
@@ -57,6 +57,9 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+	struct sev_user_data_snp_status snp_plat_status;
+	struct sev_feature_info feat_info;
 };
 
 int sev_dev_init(struct psp_device *psp);
include/linux/psp-sev.h +31 -0
index 903ddfea8585..d46d73911a76 100644
@@ -107,6 +107,7 @@ enum sev_cmd {
 	SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
 	SEV_CMD_SNP_COMMIT		= 0x0CB,
 	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
+	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
 
 	SEV_CMD_MAX,
 };

@@ -812,6 +813,36 @@ struct sev_data_snp_commit {
 	u32 len;
 } __packed;
 
+/**
+ * struct sev_snp_feature_info - SEV_SNP_FEATURE_INFO structure
+ *
+ * @length: len of the command buffer read by the PSP
+ * @ecx_in: subfunction index
+ * @feature_info_paddr : SPA of the FEATURE_INFO structure
+ */
+struct sev_snp_feature_info {
+	u32 length;
+	u32 ecx_in;
+	u64 feature_info_paddr;
+} __packed;
+
+/**
+ * struct feature_info - FEATURE_INFO structure
+ *
+ * @eax: output of SEV_SNP_FEATURE_INFO command
+ * @ebx: output of SEV_SNP_FEATURE_INFO command
+ * @ecx: output of SEV_SNP_FEATURE_INFO command
+ * #edx: output of SEV_SNP_FEATURE_INFO command
+ */
+struct sev_feature_info {
+	u32 eax;
+	u32 ebx;
+	u32 ecx;
+	u32 edx;
+} __packed;
+
+#define FEAT_CIPHERTEXTHIDING_SUPPORTED	BIT(3)
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
Signature · 1 line
2.34.1
maintainer
#4 lore raw

Re: [PATCH 2/3] crypto: ccp: Add support for SNP_FEATURE_INFO command

Quoted 7 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

The FEATURE_INFO command provides host and guests a programmatic means to learn about the supported features of the currently loaded firmware. FEATURE_INFO command leverages the same mechanism as the CPUID instruction. Instead of using the CPUID instruction to retrieve Fn8000_0024, software can use FEATURE_INFO.

Expand on this a bit. This should state that host/hypervisor would use the FEATURE_INFO command, while guests would actually issue the CPUID instruction. The idea being, that the hypervisor could add Fn8000_0024 values to the CPUID table provided to the guest.

Quoted 25 lines

During CCP module initialization, after firmware update, the SNP platform status and feature information from CPUID 0x8000_0024, sub-function 0, are cached in the sev_device structure.

Signed-off-by Ashish Kalra <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 40 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 +++
 include/linux/psp-sev.h      | 31 ++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)
drivers/crypto/ccp/sev-dev.c +1 -0
index 9810edbb272d..eefb481db5af 100644
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
+	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_feature_info);
 	default:				return 0;
 	}

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1056

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +1 -0
@@ -1052,6 +1053,43 @@ static void snp_set_hsave_pa(void *arg)
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 }
 
+static void sev_cache_snp_platform_status_and_discover_features(void)

How about snp_get_platform_info or snp_get_platform_data. If anything is ever added to this in the future, the name won't have to be changed.

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1068

Quoted 12 lines
drivers/crypto/ccp/sev-dev.c +12 -0
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct sev_snp_feature_info snp_feat_info;
+	struct sev_feature_info *feat_info;
+	struct sev_data_snp_addr buf;
+	int error = 0, rc;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	buf.address = __psp_pa(&sev->snp_plat_status);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &error);

This command is being called before SNP_INIT_EX (for now), so this is currently safe. But you should probably guard against it being called afterwards by checking the SNP state. If the SNP state is INIT, then bail or use an intermediate firmware page for output.

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1071

Quoted 3 lines
drivers/crypto/ccp/sev-dev.c +3 -0
+
+	/*
+	 * Do feature discovery of the currently loaded firmware,

Block comments in this directory start like this:

	/* Do feature discovery...

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1077

Quoted 6 lines
drivers/crypto/ccp/sev-dev.c +6 -0
+	 * and cache feature information from CPUID 0x8000_0024,
+	 * sub-function 0.
+	 */
+	if (!rc && sev->snp_plat_status.feature_info) {
+		/*
+		 * Use dynamically allocated structure for the SNP_FEATURE_INFO

Ditto.

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1088

Quoted 11 lines
drivers/crypto/ccp/sev-dev.c +11 -0
+		 * command to handle any alignment and page boundary check
+		 * requirements.
+		 */
+		feat_info = kzalloc(sizeof(*feat_info), GFP_KERNEL);
+		snp_feat_info.length = sizeof(snp_feat_info);
+		snp_feat_info.ecx_in = 0;
+		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
+
+		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, &error);
+		if (!rc)
+			sev->feat_info = *feat_info;

Above, you go directly into the sev->snp_plat_status field, but here you allocate memory and then copy. Any reason for the difference?

Quoted 44 lines
drivers/crypto/ccp/sev-dev.c +6 -0
+		kfree(feat_info);
+	}
+}
+
 static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 {
 	struct sev_data_range_list *range_list = arg;

@@ -2395,6 +2433,8 @@ void sev_pci_init(void)
 	if (sev_update_firmware(sev->dev) == 0)
 		sev_get_api_version();
 
+	sev_cache_snp_platform_status_and_discover_features();
+
 	/* Initialize the platform */
 	args.probe = true;
 	rc = sev_platform_init(&args);
drivers/crypto/ccp/sev-dev.h +3 -0
index 3e4e5574e88a..11e571e87e18 100644
@@ -57,6 +57,9 @@ struct sev_device {
 	bool cmd_buf_backup_active;
 
 	bool snp_initialized;
+
+	struct sev_user_data_snp_status snp_plat_status;
+	struct sev_feature_info feat_info;
 };
 
 int sev_dev_init(struct psp_device *psp);
include/linux/psp-sev.h +1 -0
index 903ddfea8585..d46d73911a76 100644
@@ -107,6 +107,7 @@ enum sev_cmd {
 	SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
 	SEV_CMD_SNP_COMMIT		= 0x0CB,
 	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
+	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
 
 	SEV_CMD_MAX,
 };

@@ -812,6 +813,36 @@ struct sev_data_snp_commit {
 	u32 len;

include/linux/psp-sev.h struct sev_data_snp_commit { · line 823

Quoted 10 lines
include/linux/psp-sev.h +8 -0
 } __packed;
 
+/**
+ * struct sev_snp_feature_info - SEV_SNP_FEATURE_INFO structure
+ *
+ * @length: len of the command buffer read by the PSP
+ * @ecx_in: subfunction index
+ * @feature_info_paddr : SPA of the FEATURE_INFO structure
+ */
+struct sev_snp_feature_info {

You should be consistent with the other names and call this sev_data_snp_feature_info.

include/linux/psp-sev.h struct sev_data_snp_commit { · line 835

Quoted 12 lines
include/linux/psp-sev.h +12 -0
+	u32 length;
+	u32 ecx_in;
+	u64 feature_info_paddr;
+} __packed;
+
+/**
+ * struct feature_info - FEATURE_INFO structure
+ *
+ * @eax: output of SEV_SNP_FEATURE_INFO command
+ * @ebx: output of SEV_SNP_FEATURE_INFO command
+ * @ecx: output of SEV_SNP_FEATURE_INFO command
+ * #edx: output of SEV_SNP_FEATURE_INFO command

s/SEV_//

include/linux/psp-sev.h struct sev_data_snp_commit { · line 844

Quoted 9 lines
include/linux/psp-sev.h +9 -0
+ */
+struct sev_feature_info {
+	u32 eax;
+	u32 ebx;
+	u32 ecx;
+	u32 edx;
+} __packed;
+
+#define FEAT_CIPHERTEXTHIDING_SUPPORTED	BIT(3)

How about SNP_CIPHER_TEXT_HIDING_SUPPORTED.

But, this shouldn't be defined until you use it.

Thanks,
Tom

Quoted 4 lines
include/linux/psp-sev.h +1 -0
+
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
 /**
author maintainer
#5 lore raw

PATCH 3/3 x86/sev: Add SEV-SNP CipherTextHiding support

In reply to Ashish Kalra

From: Ashish Kalra <[email protected]>

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.

Signed-off-by Ashish Kalra <[email protected]>
---
 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(-)
arch/x86/kvm/svm/sev.c +21 -3
index 532df12b43c5..954ef99a1aa8 100644
@@ -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;
+
 	/*
 	 * 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);
+	}
 	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;
drivers/crypto/ccp/sev-dev.c +50 -0

73 lines not on this page · raw message

include/linux/psp-sev.h +8 -2
index d46d73911a76..a26b43c2eab9 100644
@@ -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;
 
 /**
Signature · 1 line
2.34.1
maintainer
#6 lore raw

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

Quoted 11 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

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.

Quoted 4 lines

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.

Quoted 13 lines
Signed-off-by Ashish Kalra <[email protected]>
---
 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(-)
arch/x86/kvm/svm/sev.c +0 -0
index 532df12b43c5..954ef99a1aa8 100644

arch/x86/kvm/svm/sev.c static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) · line 177

Quoted 5 lines
arch/x86/kvm/svm/sev.c +2 -0
@@ -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?

Quoted 25 lines
arch/x86/kvm/svm/sev.c +13 -0
+
 	/*
 	 * 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" :

arch/x86/kvm/svm/sev.c void __init sev_hardware_setup(void) · line 3081

Quoted 10 lines
arch/x86/kvm/svm/sev.c +4 -2
 								       "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?

Quoted 14 lines
arch/x86/kvm/svm/sev.c +2 -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, max_snp_asid ? : min_sev_asid - 1);
 
 	sev_enabled = sev_supported;
 	sev_es_enabled = sev_es_supported;
drivers/crypto/ccp/sev-dev.c +0 -0
index eefb481db5af..9ee81a6defc5 100644

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 76

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +1 -0
@@ -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.

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 80

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +4 -0
+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.

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 91

Quoted 11 lines
drivers/crypto/ccp/sev-dev.c +6 -0
+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.

drivers/crypto/ccp/sev-dev.c static bool psp_init_on_probe = true; · line 95

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +4 -0
+
+/* MAX_SNP_ASID */
+unsigned int max_snp_asid;
+EXPORT_SYMBOL(max_snp_asid);

This should have "snp_" to provide better namespace isolation.

Quoted 12 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+
 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;

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1085

Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +10 -0
+	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?

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1089

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +4 -0
+		/* 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) {

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1090

Quoted 1 line
drivers/crypto/ccp/sev-dev.c +1 -0
+			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

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1094

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +4 -0
+				 edx - 1);
+			psp_max_snp_asid = edx - 1;
+		}
+		max_snp_asid = psp_max_snp_asid ? : (edx - 1) / 2;

Add a blank line here

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1097

Quoted 3 lines
drivers/crypto/ccp/sev-dev.c +3 -0
+		snp_cipher_text_hiding_en = 1;
+		data->ciphertext_hiding_en = 1;
+		data->max_snp_asid = max_snp_asid;

Ditto

Thanks,
Tom

Quoted 48 lines
drivers/crypto/ccp/sev-dev.c +8 -0
+		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);
include/linux/psp-sev.h +8 -2
index d46d73911a76..a26b43c2eab9 100644
@@ -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;
 
 /**
author maintainer
#7 lore raw

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

Hi Tom,

Quoted 8 lines · Tom Lendacky
Quoted 8 lines
drivers/crypto/ccp/sev-dev.c +1 -0
+
 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);
 }
Quoted 17 lines
Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +4 -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;

drivers/crypto/ccp/sev-dev.c static void snp_set_hsave_pa(void *arg) · line 1085

Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +10 -0
+	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?

FEATURE_INFO only indicates if SEV FW is capable of ciphertext hiding and SNP_PLATFORM_STATUS ciphertext_hiding_cap bit indicates if the CTH feature is enabled in BIOS, so both checks are needed.

Thanks, Ashish