2021-11-02 14:24:00

by Peter Gonda

Subject: [PATCH V3 0/4] Add SEV_INIT_EX support

SEV_INIT requires users to unlock their SPI bus for the PSP's non
volatile (NV) storage. Users may wish to lock their SPI bus for numerous
reasons, to support this the PSP firmware supports SEV_INIT_EX. INIT_EX
allows the firmware to use a region of memory for its NV storage leaving
the kernel responsible for actually storing the data in a persistent
way. This series adds a new module parameter to ccp allowing users to
specify a path to a file for use as the PSP's NV storage. The ccp driver
then reads the file into memory for the PSP to use and is responsible
for writing the file whenever the PSP modifies the memory region.

Signed-off-by: Peter Gonda <[email protected]>
Reviewed-by: Marc Orr <[email protected]>
Acked-by: David Rientjes <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: John Allen <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]

David Rientjes (1):
crypto: ccp - Add SEV_INIT_EX support

Peter Gonda (3):
crypto: ccp - Fix SEV_INIT error logging on init
crypto: ccp - Move SEV_INIT retry for corrupted data
crypto: ccp - Refactor out sev_fw_alloc()

.../virt/kvm/amd-memory-encryption.rst | 6 +
drivers/crypto/ccp/sev-dev.c | 226 +++++++++++++++---
include/linux/psp-sev.h | 21 ++
3 files changed, 221 insertions(+), 32 deletions(-)

--
2.33.1.1089.g2158813163f-goog

2021-11-02 14:24:03

by Peter Gonda

Subject: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init

Currently only the firmware error code is printed. This is incomplete
and also incorrect as error cases exists where the firmware is never
called and therefore does not set an error code. This change zeros the
firmware error code in case the call does not get that far and prints
the return code for non firmware errors.

Signed-off-by: Peter Gonda <[email protected]>
Reviewed-by: Marc Orr <[email protected]>
Acked-by: David Rientjes <[email protected]>
Acked-by: Tom Lendacky <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: John Allen <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/ccp/sev-dev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 2ecb0e1f65d8..ec89a82ba267 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1065,7 +1065,7 @@ void sev_pci_init(void)
{
struct sev_device *sev = psp_master->sev_data;
struct page *tmr_page;
- int error, rc;
+ int error = 0, rc;

if (!sev)
return;
@@ -1104,7 +1104,8 @@ void sev_pci_init(void)
}

if (rc) {
- dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error);
+ dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
+ error, rc);
return;
}

--
2.33.1.1089.g2158813163f-goog

2021-11-02 14:24:05

by Peter Gonda

Subject: [PATCH V3 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

This change moves the data corrupted retry of SEV_INIT into the
__sev_platform_init_locked() function. This is for upcoming INIT_EX
support as well as helping direct callers of
__sev_platform_init_locked() which currently do not support the
retry.

Signed-off-by: Peter Gonda <[email protected]>
Reviewed-by: Marc Orr <[email protected]>
Acked-by: David Rientjes <[email protected]>
Acked-by: Tom Lendacky <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: John Allen <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ec89a82ba267..e4bc833949a0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
}

rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+ if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
+ /*
+ * INIT command returned an integrity check failure
+ * status code, meaning that firmware load and
+ * validation of SEV related persistent data has
+ * failed and persistent state has been erased.
+ * Retrying INIT command here should succeed.
+ */
+ dev_dbg(sev->dev, "SEV: retrying INIT command");
+ rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+ }
+
if (rc)
return rc;

@@ -1091,18 +1103,6 @@ void sev_pci_init(void)

/* Initialize the platform */
rc = sev_platform_init(&error);
- if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
- /*
- * INIT command returned an integrity check failure
- * status code, meaning that firmware load and
- * validation of SEV related persistent data has
- * failed and persistent state has been erased.
- * Retrying INIT command here should succeed.
- */
- dev_dbg(sev->dev, "SEV: retrying INIT command");
- rc = sev_platform_init(&error);
- }
-
if (rc) {
dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
error, rc);
--
2.33.1.1089.g2158813163f-goog

2021-11-02 14:24:07

by Peter Gonda

Subject: [PATCH V3 3/4] crypto: ccp - Refactor out sev_fw_alloc()

Creates a helper function sev_fw_alloc() which can be used to allocate
aligned memory regions for use by the PSP firmware. Currently only used
for the SEV-ES TMR region but will be used for the SEV_INIT_EX NV memory
region.

Signed-off-by: Peter Gonda <[email protected]>
Reviewed-by: Marc Orr <[email protected]>
Acked-by: David Rientjes <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: John Allen <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/ccp/sev-dev.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e4bc833949a0..00ca74dd7b3c 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -141,6 +141,17 @@ static int sev_cmd_buffer_len(int cmd)
return 0;
}

+static void *sev_fw_alloc(unsigned long len)
+{
+ struct page *page;
+
+ page = alloc_pages(GFP_KERNEL, get_order(len));
+ if (!page)
+ return NULL;
+
+ return page_address(page);
+}
+
static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
{
struct psp_device *psp = psp_master;
@@ -1076,7 +1087,6 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
void sev_pci_init(void)
{
struct sev_device *sev = psp_master->sev_data;
- struct page *tmr_page;
int error = 0, rc;

if (!sev)
@@ -1092,14 +1102,10 @@ void sev_pci_init(void)
sev_get_api_version();

/* Obtain the TMR memory area for SEV-ES use */
- tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE));
- if (tmr_page) {
- sev_es_tmr = page_address(tmr_page);
- } else {
- sev_es_tmr = NULL;
+ sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
+ if (!sev_es_tmr)
dev_warn(sev->dev,
"SEV: TMR allocation failed, SEV-ES support unavailable\n");
- }

/* Initialize the platform */
rc = sev_platform_init(&error);
--
2.33.1.1089.g2158813163f-goog

2021-11-02 14:24:09

by Peter Gonda

Subject: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

From: David Rientjes <[email protected]>

Add new module parameter to allow users to use SEV_INIT_EX instead of
SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
functionality. The 'init_ex_path' parameter defaults to NULL which means
the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
used with the data found at the path. On certain PSP commands this
file is written to as the PSP updates the NV memory region. Depending on
file system initialization this file open may fail during module init
but the CCP driver for SEV already has sufficient retries for platform
initialization. During normal operation of PSP system and SEV commands
if the PSP has not been initialized it is at run time. If the file at
'init_ex_path' does not exist the PSP will not be initialized. The user
must create the file prior to use with 32Kb of 0xFFs per spec.

Signed-off-by: David Rientjes <[email protected]>
Co-developed-by: Peter Gonda <[email protected]>
Signed-off-by: Peter Gonda <[email protected]>
Reviewed-by: Marc Orr <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Brijesh Singh <[email protected]>
Cc: Marc Orr <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: John Allen <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
.../virt/kvm/amd-memory-encryption.rst | 6 +
drivers/crypto/ccp/sev-dev.c | 181 ++++++++++++++++--
include/linux/psp-sev.h | 21 ++
3 files changed, 195 insertions(+), 13 deletions(-)

diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
index 5c081c8c7164..1c6847fff304 100644
--- a/Documentation/virt/kvm/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/amd-memory-encryption.rst
@@ -85,6 +85,12 @@ guests, such as launching, running, snapshotting, migrating and decommissioning.
The KVM_SEV_INIT command is used by the hypervisor to initialize the SEV platform
context. In a typical workflow, this command should be the first command issued.

+The firmware can be initialized either by using its own non-volatile storage or
+the OS can manage the NV storage for the firmware using the module parameter
+``init_ex_path``. The file specified by ``init_ex_path`` must exist. To create
+a new NV storage file allocate the file with 32KB bytes of 0xFF as required by
+the SEV spec.
+
Returns: 0 on success, -negative on error

2. KVM_SEV_LAUNCH_START
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 00ca74dd7b3c..2264a0b76bee 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -22,6 +22,7 @@
#include <linux/firmware.h>
#include <linux/gfp.h>
#include <linux/cpufeature.h>
+#include <linux/fs.h>

#include <asm/smp.h>

@@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
module_param(psp_probe_timeout, int, 0644);
MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");

+static char *init_ex_path;
+module_param(init_ex_path, charp, 0660);
+MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
+
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 */
@@ -58,6 +63,14 @@ static int psp_timeout;
#define SEV_ES_TMR_SIZE (1024 * 1024)
static void *sev_es_tmr;

+/* INIT_EX NV Storage:
+ * The NV Storage is a 32Kb area and must be 4Kb page aligned. Use the page
+ * allocator to allocate the memory, which will return aligned memory for the
+ * specified allocation order.
+ */
+#define NV_LENGTH (32 * 1024)
+static void *sev_init_ex_nv_address;
+
static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
{
struct sev_device *sev = psp_master->sev_data;
@@ -107,6 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
{
switch (cmd) {
case SEV_CMD_INIT: return sizeof(struct sev_data_init);
+ case SEV_CMD_INIT_EX: return sizeof(struct sev_data_init_ex);
case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_user_data_status);
case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr);
case SEV_CMD_PEK_CERT_IMPORT: return sizeof(struct sev_data_pek_cert_import);
@@ -152,6 +166,87 @@ static void *sev_fw_alloc(unsigned long len)
return page_address(page);
}

+static int sev_read_nv_memory(void)
+{
+ struct file *fp;
+ ssize_t nread;
+
+ if (!sev_init_ex_nv_address)
+ return -EOPNOTSUPP;
+
+ fp = filp_open(init_ex_path, O_RDONLY, 0);
+ if (IS_ERR(fp)) {
+ int ret = PTR_ERR(fp);
+
+ dev_err(psp_master->dev,
+ "SEV: could not open %s for read, error %d\n",
+ init_ex_path, ret);
+ return ret;
+ }
+
+ nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
+ dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
+ filp_close(fp, NULL);
+
+ return 0;
+}
+
+static void sev_write_nv_memory(void)
+{
+ struct sev_device *sev = psp_master->sev_data;
+ struct file *fp;
+ loff_t offset = 0;
+ ssize_t nwrite;
+
+ if (!sev_init_ex_nv_address)
+ return;
+
+ fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
+ if (IS_ERR(fp)) {
+ dev_err(sev->dev,
+ "SEV: could not open file for write, error %d\n",
+ PTR_ERR(fp));
+ return;
+ }
+
+ nwrite = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
+ vfs_fsync(fp, 0);
+ filp_close(fp, NULL);
+
+ if (nwrite != NV_LENGTH) {
+ dev_err(sev->dev,
+ "SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
+ NV_LENGTH, nwrite);
+ return;
+ }
+
+ dev_dbg(sev->dev, "SEV: write successful to NV file\n");
+}
+
+static void sev_write_nv_memory_if_required(int cmd_id)
+{
+ if (!sev_init_ex_nv_address)
+ return;
+
+ /*
+ * Only a few platform commands modify the SPI/NV area, but none of the
+ * non-platform commands do. Only INIT(_EX), PLATFORM_RESET, PEK_GEN,
+ * PEK_CERT_IMPORT, and PDH_GEN do.
+ */
+ switch (cmd_id) {
+ case SEV_CMD_FACTORY_RESET:
+ case SEV_CMD_INIT_EX:
+ case SEV_CMD_PDH_GEN:
+ case SEV_CMD_PEK_CERT_IMPORT:
+ case SEV_CMD_PEK_GEN:
+ break;
+ default:
+ return;
+ };
+
+ sev_write_nv_memory();
+}
+
static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
{
struct psp_device *psp = psp_master;
@@ -221,6 +316,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
cmd, reg & PSP_CMDRESP_ERR_MASK);
ret = -EIO;
+ } else {
+ sev_write_nv_memory_if_required(cmd);
}

print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
@@ -247,22 +344,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
return rc;
}

-static int __sev_platform_init_locked(int *error)
+static int __sev_init_locked(int *error)
{
- struct psp_device *psp = psp_master;
struct sev_data_init data;
- struct sev_device *sev;
- int rc = 0;

- if (!psp || !psp->sev_data)
- return -ENODEV;
+ memset(&data, 0, sizeof(data));
+ if (sev_es_tmr) {
+ u64 tmr_pa;

- sev = psp->sev_data;
+ /*
+ * Do not include the encryption mask on the physical
+ * address of the TMR (firmware should clear it anyway).
+ */
+ tmr_pa = __pa(sev_es_tmr);

- if (sev->state == SEV_STATE_INIT)
- return 0;
+ data.flags |= SEV_INIT_FLAGS_SEV_ES;
+ data.tmr_address = tmr_pa;
+ data.tmr_len = SEV_ES_TMR_SIZE;
+ }
+
+ return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+}
+
+static int __sev_init_ex_locked(int *error)
+{
+ struct sev_data_init_ex data;
+ int ret;

memset(&data, 0, sizeof(data));
+ data.length = sizeof(data);
+ data.nv_address = __psp_pa(sev_init_ex_nv_address);
+ data.nv_len = NV_LENGTH;
+
+ ret = sev_read_nv_memory();
+ if (ret)
+ return ret;
+
if (sev_es_tmr) {
u64 tmr_pa;

@@ -277,7 +394,27 @@ static int __sev_platform_init_locked(int *error)
data.tmr_len = SEV_ES_TMR_SIZE;
}

- rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+ return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
+}
+
+static int __sev_platform_init_locked(int *error)
+{
+ struct psp_device *psp = psp_master;
+ struct sev_device *sev;
+ int rc;
+ int (*init_function)(int *error);
+
+ if (!psp || !psp->sev_data)
+ return -ENODEV;
+
+ sev = psp->sev_data;
+
+ if (sev->state == SEV_STATE_INIT)
+ return 0;
+
+ init_function = sev_init_ex_nv_address ? __sev_init_ex_locked :
+ __sev_init_locked;
+ rc = init_function(error);
if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
/*
* INIT command returned an integrity check failure
@@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
* failed and persistent state has been erased.
* Retrying INIT command here should succeed.
*/
- dev_dbg(sev->dev, "SEV: retrying INIT command");
- rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+ dev_notice(sev->dev, "SEV: retrying INIT command");
+ rc = init_function(error);
}

if (rc)
@@ -303,7 +440,7 @@ static int __sev_platform_init_locked(int *error)

dev_dbg(sev->dev, "SEV firmware initialized\n");

- return rc;
+ return 0;
}

int sev_platform_init(int *error)
@@ -1057,6 +1194,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
get_order(SEV_ES_TMR_SIZE));
sev_es_tmr = NULL;
}
+
+ if (sev_init_ex_nv_address) {
+ free_pages((unsigned long)sev_init_ex_nv_address,
+ get_order(NV_LENGTH));
+ sev_init_ex_nv_address = NULL;
+ }
}

void sev_dev_destroy(struct psp_device *psp)
@@ -1101,6 +1244,18 @@ void sev_pci_init(void)
sev_update_firmware(sev->dev) == 0)
sev_get_api_version();

+ /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
+ * instead of INIT.
+ */
+ if (init_ex_path) {
+ sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
+ if (!sev_init_ex_nv_address) {
+ dev_err(sev->dev,
+ "SEV: INIT_EX NV memory allocation failed\n");
+ goto err;
+ }
+ }
+
/* Obtain the TMR memory area for SEV-ES use */
sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
if (!sev_es_tmr)
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index d48a7192e881..1595088c428b 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -52,6 +52,7 @@ enum sev_cmd {
SEV_CMD_DF_FLUSH = 0x00A,
SEV_CMD_DOWNLOAD_FIRMWARE = 0x00B,
SEV_CMD_GET_ID = 0x00C,
+ SEV_CMD_INIT_EX = 0x00D,

/* Guest commands */
SEV_CMD_DECOMMISSION = 0x020,
@@ -102,6 +103,26 @@ struct sev_data_init {
u32 tmr_len; /* In */
} __packed;

+/**
+ * struct sev_data_init_ex - INIT_EX command parameters
+ *
+ * @length: len of the command buffer read by the PSP
+ * @flags: processing flags
+ * @tmr_address: system physical address used for SEV-ES
+ * @tmr_len: len of tmr_address
+ * @nv_address: system physical address used for PSP NV storage
+ * @nv_len: len of nv_address
+ */
+struct sev_data_init_ex {
+ u32 length; /* In */
+ u32 flags; /* In */
+ u64 tmr_address; /* In */
+ u32 tmr_len; /* In */
+ u32 reserved; /* In */
+ u64 nv_address; /* In/Out */
+ u32 nv_len; /* In */
+} __packed;
+
#define SEV_INIT_FLAGS_SEV_ES 0x01

/**
--
2.33.1.1089.g2158813163f-goog

2021-11-02 15:42:41

by Tom Lendacky

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On 11/2/21 9:23 AM, Peter Gonda wrote:
> From: David Rientjes <[email protected]>
>
> +
> + nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);

Not sure if you missed the previous comment, but kernel_read can return an
error, shouldn't it be checked and fail on error?

Thanks,
Tom

> + dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> + filp_close(fp, NULL);
> +
> + return 0;
> +}
> +

2021-11-02 16:37:16

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Tue, Nov 2, 2021 at 9:38 AM Tom Lendacky <[email protected]> wrote:
>
> On 11/2/21 9:23 AM, Peter Gonda wrote:
> > From: David Rientjes <[email protected]>
> >
> > +
> > + nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
>
> Not sure if you missed the previous comment, but kernel_read can return an
> error, shouldn't it be checked and fail on error?

I did miss that comment. Updated to make sure nread == NV_LENGTH.
>
> Thanks,
> Tom
>
> > + dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> > + filp_close(fp, NULL);
> > +
> > + return 0;
> > +}
> > +

2021-11-02 16:40:00

by Sean Christopherson

Subject: Re: [PATCH V3 0/4] Add SEV_INIT_EX support

On Tue, Nov 02, 2021, Peter Gonda wrote:
> SEV_INIT requires users to unlock their SPI bus for the PSP's non
> volatile (NV) storage. Users may wish to lock their SPI bus for numerous
> reasons, to support this the PSP firmware supports SEV_INIT_EX. INIT_EX
> allows the firmware to use a region of memory for its NV storage leaving
> the kernel responsible for actually storing the data in a persistent
> way. This series adds a new module parameter to ccp allowing users to
> specify a path to a file for use as the PSP's NV storage. The ccp driver
> then reads the file into memory for the PSP to use and is responsible
> for writing the file whenever the PSP modifies the memory region.

What's changed between v1 and v3? Also, please wait a few days between versions.
I know us KVM people are often slow to get to reviews, but posting a new version
every day is usually counter-productive as it increases the review cost (more
threads to find and read).

2021-11-02 16:50:11

by Peter Gonda

Subject: Re: [PATCH V3 0/4] Add SEV_INIT_EX support

On Tue, Nov 2, 2021 at 10:05 AM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Nov 02, 2021, Peter Gonda wrote:
> > SEV_INIT requires users to unlock their SPI bus for the PSP's non
> > volatile (NV) storage. Users may wish to lock their SPI bus for numerous
> > reasons, to support this the PSP firmware supports SEV_INIT_EX. INIT_EX
> > allows the firmware to use a region of memory for its NV storage leaving
> > the kernel responsible for actually storing the data in a persistent
> > way. This series adds a new module parameter to ccp allowing users to
> > specify a path to a file for use as the PSP's NV storage. The ccp driver
> > then reads the file into memory for the PSP to use and is responsible
> > for writing the file whenever the PSP modifies the memory region.
>
> What's changed between v1 and v3? Also, please wait a few days between versions.
> I know us KVM people are often slow to get to reviews, but posting a new version
> every day is usually counter-productive as it increases the review cost (more
> threads to find and read).

My mistake. I can wait longer between revisions. I was just trying to
include Tom's feedback promptly, I didn't think having many versions
would be an issue.

Between V1 and V3: I have fixed a lot of style issues Tom identified.
Added documentation to the SEV documentation file. Fixed some
incorrect type usage. Made the logging more uniform. Removed writing
on the SHUTDOWN command. And fixed some error handling.

2021-11-09 22:33:33

by Sean Christopherson

Subject: Re: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init

On Tue, Nov 02, 2021, Peter Gonda wrote:
> Currently only the firmware error code is printed. This is incomplete
> and also incorrect as error cases exists where the firmware is never
> called and therefore does not set an error code. This change zeros the
> firmware error code in case the call does not get that far and prints
> the return code for non firmware errors.
>
> Signed-off-by: Peter Gonda <[email protected]>
> Reviewed-by: Marc Orr <[email protected]>
> Acked-by: David Rientjes <[email protected]>
> Acked-by: Tom Lendacky <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Brijesh Singh <[email protected]>
> Cc: Marc Orr <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: John Allen <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/crypto/ccp/sev-dev.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2ecb0e1f65d8..ec89a82ba267 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1065,7 +1065,7 @@ void sev_pci_init(void)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct page *tmr_page;
> - int error, rc;
> + int error = 0, rc;

Wouldn't it be more appropriate to use something the PSP can't return, e.g. -1?
'0' is SEV_RET_SUCCESS, which is quite misleading, e.g. the below error message
will print

SEV: failed to INIT error 0, rc -16

which a bit head scratching without looking at the code. AFAICT, the PSP return
codes aren't intrinsically hex, so printing error as a signed demical and thus

SEV: failed to INIT error -1, rc -16

would be less confusing.

And IMO requiring the caller to initialize error is will be neverending game of
whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure,
and literally every function exposed via include/linux/psp-sev.h has this same
issue. Case in point, the retry path fails to re-initialize "error" and will
display stale information if the second sev_platform_init() fails without reaching
the PSP.

rc = sev_platform_init(&error);
if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
/*
* INIT command returned an integrity check failure
* status code, meaning that firmware load and
* validation of SEV related persistent data has
* failed and persistent state has been erased.
* Retrying INIT command here should succeed.
*/
dev_dbg(sev->dev, "SEV: retrying INIT command");
rc = sev_platform_init(&error); <------ error may or may not be set
}

Ideally, error wouldn't be an output param and instead would be squished into the
true return value, but that'd required quite the refactoring, and might yield ugly
code generation on non-64-bit architectures (does this code support those?).

As a minimal step toward sanity, sev_ioctl(), __sev_platform_init_locked(), and
__sev_do_cmd_locked() should initialize the incoming error. Long term, sev-dev
really needs to either have well-defined API for when "error" is meaningful, or
ensure the pointer is initialized in all paths.

E.g.

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ec89a82ba267..549686a1e812 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -149,6 +149,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
unsigned int reg, ret = 0;
int buf_len;

+ if (psp_ret)
+ *psp_ret = -1;
+
if (!psp || !psp->sev_data)
return -ENODEV;

@@ -192,9 +195,6 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
/* wait for command completion */
ret = sev_wait_cmd_ioc(sev, &reg, psp_timeout);
if (ret) {
- if (psp_ret)
- *psp_ret = 0;
-
dev_err(sev->dev, "sev command %#x timed out, disabling PSP\n", cmd);
psp_dead = true;

@@ -243,6 +243,9 @@ static int __sev_platform_init_locked(int *error)
struct sev_device *sev;
int rc = 0;

+ if (error)
+ *error = -1;
+
if (!psp || !psp->sev_data)
return -ENODEV;

@@ -838,6 +841,8 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
if (input.cmd > SEV_MAX)
return -EINVAL;

+ input.error = -1;
+
mutex_lock(&sev_cmd_mutex);

switch (input.cmd) {

> if (!sev)
> return;
> @@ -1104,7 +1104,8 @@ void sev_pci_init(void)
> }
>
> if (rc) {
> - dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error);
> + dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> + error, rc);
> return;
> }
>
> --
> 2.33.1.1089.g2158813163f-goog
>

2021-11-09 22:35:03

by Sean Christopherson

Subject: Re: [PATCH V3 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

On Tue, Nov 02, 2021, Peter Gonda wrote:
> This change moves the data corrupted retry of SEV_INIT into the

Use imperative mood.

> __sev_platform_init_locked() function. This is for upcoming INIT_EX
> support as well as helping direct callers of
> __sev_platform_init_locked() which currently do not support the
> retry.
>
> Signed-off-by: Peter Gonda <[email protected]>
> Reviewed-by: Marc Orr <[email protected]>
> Acked-by: David Rientjes <[email protected]>
> Acked-by: Tom Lendacky <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Brijesh Singh <[email protected]>
> Cc: Marc Orr <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: John Allen <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index ec89a82ba267..e4bc833949a0 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
> }
>
> rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> + if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {

There are no guarantees that @error is non-NULL as this is reachable via an
exported function, sev_platform_init(). Which ties in with my complaints in the
previous patch that the API is a bit of a mess.

> + /*
> + * INIT command returned an integrity check failure
> + * status code, meaning that firmware load and
> + * validation of SEV related persistent data has
> + * failed and persistent state has been erased.
> + * Retrying INIT command here should succeed.
> + */
> + dev_dbg(sev->dev, "SEV: retrying INIT command");
> + rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> + }
> +
> if (rc)
> return rc;
>
> @@ -1091,18 +1103,6 @@ void sev_pci_init(void)
>
> /* Initialize the platform */
> rc = sev_platform_init(&error);
> - if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
> - /*
> - * INIT command returned an integrity check failure
> - * status code, meaning that firmware load and
> - * validation of SEV related persistent data has
> - * failed and persistent state has been erased.
> - * Retrying INIT command here should succeed.
> - */
> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> - rc = sev_platform_init(&error);
> - }
> -
> if (rc) {
> dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> error, rc);
> --
> 2.33.1.1089.g2158813163f-goog
>

2021-11-09 22:36:43

by Peter Gonda

Subject: Re: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init

On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Nov 02, 2021, Peter Gonda wrote:
> > Currently only the firmware error code is printed. This is incomplete
> > and also incorrect as error cases exists where the firmware is never
> > called and therefore does not set an error code. This change zeros the
> > firmware error code in case the call does not get that far and prints
> > the return code for non firmware errors.
> >
> > Signed-off-by: Peter Gonda <[email protected]>
> > Reviewed-by: Marc Orr <[email protected]>
> > Acked-by: David Rientjes <[email protected]>
> > Acked-by: Tom Lendacky <[email protected]>
> > Cc: Tom Lendacky <[email protected]>
> > Cc: Brijesh Singh <[email protected]>
> > Cc: Marc Orr <[email protected]>
> > Cc: Joerg Roedel <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: David Rientjes <[email protected]>
> > Cc: John Allen <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/crypto/ccp/sev-dev.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 2ecb0e1f65d8..ec89a82ba267 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -1065,7 +1065,7 @@ void sev_pci_init(void)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > struct page *tmr_page;
> > - int error, rc;
> > + int error = 0, rc;
>
> Wouldn't it be more appropriate to use something the PSP can't return, e.g. -1?
> '0' is SEV_RET_SUCCESS, which is quite misleading, e.g. the below error message
> will print
>
> SEV: failed to INIT error 0, rc -16
>
> which a bit head scratching without looking at the code. AFAICT, the PSP return
> codes aren't intrinsically hex, so printing error as a signed demical and thus
>
> SEV: failed to INIT error -1, rc -16
>
> would be less confusing.
>
> And IMO requiring the caller to initialize error is will be neverending game of
> whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure,
> and literally every function exposed via include/linux/psp-sev.h has this same
> issue. Case in point, the retry path fails to re-initialize "error" and will
> display stale information if the second sev_platform_init() fails without reaching
> the PSP.

OK I can update __sev_platform_init_locked() to set error to -1. That
seems pretty reasonable. Tom, is that OK with you?

>
> rc = sev_platform_init(&error);
> if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
> /*
> * INIT command returned an integrity check failure
> * status code, meaning that firmware load and
> * validation of SEV related persistent data has
> * failed and persistent state has been erased.
> * Retrying INIT command here should succeed.
> */
> dev_dbg(sev->dev, "SEV: retrying INIT command");
> rc = sev_platform_init(&error); <------ error may or may not be set
> }
>
> Ideally, error wouldn't be an output param and instead would be squished into the
> true return value, but that'd required quite the refactoring, and might yield ugly
> code generation on non-64-bit architectures (does this code support those?).
>
> As a minimal step toward sanity, sev_ioctl(), __sev_platform_init_locked(), and
> __sev_do_cmd_locked() should initialize the incoming error. Long term, sev-dev
> really needs to either have well-defined API for when "error" is meaningful, or
> ensure the pointer is initialized in all paths.

These comments seem fine to me. But I'll refrain from updating
anything here since this seems out-of-scope of this series. Happy to
discuss further and work on that if Tom is interested in those
refactors too.

>
> E.g.
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index ec89a82ba267..549686a1e812 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -149,6 +149,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> unsigned int reg, ret = 0;
> int buf_len;
>
> + if (psp_ret)
> + *psp_ret = -1;
> +
> if (!psp || !psp->sev_data)
> return -ENODEV;
>
> @@ -192,9 +195,6 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> /* wait for command completion */
> ret = sev_wait_cmd_ioc(sev, &reg, psp_timeout);
> if (ret) {
> - if (psp_ret)
> - *psp_ret = 0;
> -
> dev_err(sev->dev, "sev command %#x timed out, disabling PSP\n", cmd);
> psp_dead = true;
>
> @@ -243,6 +243,9 @@ static int __sev_platform_init_locked(int *error)
> struct sev_device *sev;
> int rc = 0;
>
> + if (error)
> + *error = -1;
> +
> if (!psp || !psp->sev_data)
> return -ENODEV;
>
> @@ -838,6 +841,8 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> if (input.cmd > SEV_MAX)
> return -EINVAL;
>
> + input.error = -1;
> +
> mutex_lock(&sev_cmd_mutex);
>
> switch (input.cmd) {
>
> > if (!sev)
> > return;
> > @@ -1104,7 +1104,8 @@ void sev_pci_init(void)
> > }
> >
> > if (rc) {
> > - dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error);
> > + dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> > + error, rc);
> > return;
> > }
> >
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

2021-11-09 22:36:53

by Peter Gonda

Subject: Re: [PATCH V3 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Nov 02, 2021, Peter Gonda wrote:
> > This change moves the data corrupted retry of SEV_INIT into the
>
> Use imperative mood.

Will do for next revision

>
> > __sev_platform_init_locked() function. This is for upcoming INIT_EX
> > support as well as helping direct callers of
> > __sev_platform_init_locked() which currently do not support the
> > retry.
> >
> > Signed-off-by: Peter Gonda <[email protected]>
> > Reviewed-by: Marc Orr <[email protected]>
> > Acked-by: David Rientjes <[email protected]>
> > Acked-by: Tom Lendacky <[email protected]>
> > Cc: Tom Lendacky <[email protected]>
> > Cc: Brijesh Singh <[email protected]>
> > Cc: Marc Orr <[email protected]>
> > Cc: Joerg Roedel <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: David Rientjes <[email protected]>
> > Cc: John Allen <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index ec89a82ba267..e4bc833949a0 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
> > }
> >
> > rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > + if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>
> There are no guarantees that @error is non-NULL as this is reachable via an
> exported function, sev_platform_init(). Which ties in with my complaints in the
> previous patch that the API is a bit of a mess.

That seems like a bug from the caller right? Is it typical that we
sanity-check the caller in these instances? For example the same
comment could be made here:
https://elixir.bootlin.com/linux/latest/source/drivers/crypto/ccp/sev-dev.c#L336

```
static int sev_get_platform_state(int *state, int *error)
{
struct sev_user_data_status data;
int rc;

rc = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, error);
if (rc)
return rc;

*state = data.state; <--- State could be null.
return rc;
}
```

Example outside of this driver:
https://elixir.bootlin.com/linux/v5.15.1/source/arch/x86/kvm/x86.c#L468

```
int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); <---
msr_info could be null here
u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);

if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
return 1;
if (!msr_info->host_initiated) {
if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
return 1;
if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
return 1;
}

kvm_lapic_set_base(vcpu, msr_info->data);
kvm_recalculate_apic_map(vcpu->kvm);
return 0;
}
EXPORT_SYMBOL_GPL(kvm_set_apic_base);
```

About the API being a mess that seems a little out of scope for this
change. I am not changing the API surface at all here. Again happy to
discuss improvements with you and Tom for follow up series.

>
> > + /*
> > + * INIT command returned an integrity check failure
> > + * status code, meaning that firmware load and
> > + * validation of SEV related persistent data has
> > + * failed and persistent state has been erased.
> > + * Retrying INIT command here should succeed.
> > + */
> > + dev_dbg(sev->dev, "SEV: retrying INIT command");
> > + rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > + }
> > +
> > if (rc)
> > return rc;
> >
> > @@ -1091,18 +1103,6 @@ void sev_pci_init(void)
> >
> > /* Initialize the platform */
> > rc = sev_platform_init(&error);
> > - if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
> > - /*
> > - * INIT command returned an integrity check failure
> > - * status code, meaning that firmware load and
> > - * validation of SEV related persistent data has
> > - * failed and persistent state has been erased.
> > - * Retrying INIT command here should succeed.
> > - */
> > - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > - rc = sev_platform_init(&error);
> > - }
> > -
> > if (rc) {
> > dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> > error, rc);
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

2021-11-09 22:38:11

by Sean Christopherson

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Tue, Nov 02, 2021, Peter Gonda wrote:
> @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
> module_param(psp_probe_timeout, int, 0644);
> MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
>
> +static char *init_ex_path;
> +module_param(init_ex_path, charp, 0660);

Why is this writable after the module loads? At best, it seems like it will give
userspace an easy way to shoot itself in the foot, at worst it will lead to a
TOCTOU bug in the kernel.

> +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
> +
> 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 */
> @@ -58,6 +63,14 @@ static int psp_timeout;
> #define SEV_ES_TMR_SIZE (1024 * 1024)
> static void *sev_es_tmr;
>
> +/* INIT_EX NV Storage:
> + * The NV Storage is a 32Kb area and must be 4Kb page aligned. Use the page
> + * allocator to allocate the memory, which will return aligned memory for the
> + * specified allocation order.
> + */
> +#define NV_LENGTH (32 * 1024)
> +static void *sev_init_ex_nv_address;

The "address" part is redundant and potentially confusing, e.g. one might expect
it to contain a physical address.

And the "NV" part is kind of a lie, this isn't in non-volatile memory (it's a
kernel buffer) and it's obviously volatile in the sense that it can be changed
by the PSP. I get that from the PSP's perspective it's _intended_ to be NV
storage, but (a) this does not point at NV storage and (b) there is no guarantee
that the path provided by userspace points at NV storage.

Maybe "sev_init_ex_buffer" or something along those lines?

> static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> {
> struct sev_device *sev = psp_master->sev_data;
> @@ -107,6 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
> {
> switch (cmd) {
> case SEV_CMD_INIT: return sizeof(struct sev_data_init);
> + case SEV_CMD_INIT_EX: return sizeof(struct sev_data_init_ex);
> case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_user_data_status);
> case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr);
> case SEV_CMD_PEK_CERT_IMPORT: return sizeof(struct sev_data_pek_cert_import);
> @@ -152,6 +166,87 @@ static void *sev_fw_alloc(unsigned long len)
> return page_address(page);
> }
>
> +static int sev_read_nv_memory(void)

Similar to above, this is reading from a file that's backed by who knows what,
i.e. it may or may not be reading NV memory. And if it's going to implicitly
read into the buffer, that should be reflected in the name.

> +{
> + struct file *fp;
> + ssize_t nread;
> +
> + if (!sev_init_ex_nv_address)
> + return -EOPNOTSUPP;
> +
> + fp = filp_open(init_ex_path, O_RDONLY, 0);
> + if (IS_ERR(fp)) {
> + int ret = PTR_ERR(fp);
> +
> + dev_err(psp_master->dev,
> + "SEV: could not open %s for read, error %d\n",
> + init_ex_path, ret);
> + return ret;
> + }
> +
> + nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
> + dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> + filp_close(fp, NULL);
> +
> + return 0;
> +}
> +
> +static void sev_write_nv_memory(void)

Same comments here.

> +{
> + struct sev_device *sev = psp_master->sev_data;
> + struct file *fp;
> + loff_t offset = 0;
> + ssize_t nwrite;
> +
> + if (!sev_init_ex_nv_address)
> + return;
> +
> + fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> + if (IS_ERR(fp)) {
> + dev_err(sev->dev,
> + "SEV: could not open file for write, error %d\n",
> + PTR_ERR(fp));
> + return;
> + }
> +
> + nwrite = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> + vfs_fsync(fp, 0);
> + filp_close(fp, NULL);
> +
> + if (nwrite != NV_LENGTH) {
> + dev_err(sev->dev,
> + "SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
> + NV_LENGTH, nwrite);
> + return;
> + }
> +
> + dev_dbg(sev->dev, "SEV: write successful to NV file\n");
> +}
> +
> +static void sev_write_nv_memory_if_required(int cmd_id)

And here.

> +{
> + if (!sev_init_ex_nv_address)
> + return;
> +
> + /*
> + * Only a few platform commands modify the SPI/NV area, but none of the
> + * non-platform commands do. Only INIT(_EX), PLATFORM_RESET, PEK_GEN,
> + * PEK_CERT_IMPORT, and PDH_GEN do.
> + */
> + switch (cmd_id) {
> + case SEV_CMD_FACTORY_RESET:
> + case SEV_CMD_INIT_EX:
> + case SEV_CMD_PDH_GEN:
> + case SEV_CMD_PEK_CERT_IMPORT:
> + case SEV_CMD_PEK_GEN:
> + break;
> + default:
> + return;
> + };
> +
> + sev_write_nv_memory();
> +}
> +
> static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> {
> struct psp_device *psp = psp_master;
> @@ -221,6 +316,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
> cmd, reg & PSP_CMDRESP_ERR_MASK);
> ret = -EIO;
> + } else {
> + sev_write_nv_memory_if_required(cmd);
> }
>
> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> @@ -247,22 +344,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
> return rc;
> }
>
> -static int __sev_platform_init_locked(int *error)
> +static int __sev_init_locked(int *error)
> {
> - struct psp_device *psp = psp_master;
> struct sev_data_init data;
> - struct sev_device *sev;
> - int rc = 0;
>
> - if (!psp || !psp->sev_data)
> - return -ENODEV;
> + memset(&data, 0, sizeof(data));
> + if (sev_es_tmr) {
> + u64 tmr_pa;
>
> - sev = psp->sev_data;
> + /*
> + * Do not include the encryption mask on the physical
> + * address of the TMR (firmware should clear it anyway).
> + */
> + tmr_pa = __pa(sev_es_tmr);

I realize this is copy-pasted from existing code, by why bother with an intermediate
tmr_pa? Just set data.tmr_address directly.

>
> - if (sev->state == SEV_STATE_INIT)
> - return 0;
> + data.flags |= SEV_INIT_FLAGS_SEV_ES;
> + data.tmr_address = tmr_pa;
> + data.tmr_len = SEV_ES_TMR_SIZE;
> + }
> +
> + return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +}
> +
> +static int __sev_init_ex_locked(int *error)
> +{
> + struct sev_data_init_ex data;
> + int ret;
>
> memset(&data, 0, sizeof(data));
> + data.length = sizeof(data);
> + data.nv_address = __psp_pa(sev_init_ex_nv_address);
> + data.nv_len = NV_LENGTH;
> +
> + ret = sev_read_nv_memory();

Why defer reading the file until INIT_EX? Why not read the data when the path
is first verified and consumed? More comments below in the retry path.

And what happens when sev_platform_init() is reached through a path other than
sev_pci_init()? I guess that's a question for the existing code since sev_es_tmr
will be NULL unless sev_platform_init() routes through sev_pci_init(). So either
the sev_platform_init() call from KVM's sev_guest_init() is pointless, or there's
potential for some truly bizarre and confusing behavior.

> + if (ret)
> + return ret;
> +
> if (sev_es_tmr) {
> u64 tmr_pa;
>
> @@ -277,7 +394,27 @@ static int __sev_platform_init_locked(int *error)
> data.tmr_len = SEV_ES_TMR_SIZE;
> }
>
> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> + return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
> +}
> +
> +static int __sev_platform_init_locked(int *error)
> +{
> + struct psp_device *psp = psp_master;
> + struct sev_device *sev;
> + int rc;
> + int (*init_function)(int *error);
> +
> + if (!psp || !psp->sev_data)
> + return -ENODEV;
> +
> + sev = psp->sev_data;
> +
> + if (sev->state == SEV_STATE_INIT)
> + return 0;
> +
> + init_function = sev_init_ex_nv_address ? __sev_init_ex_locked :
> + __sev_init_locked;

There's no need for this to be a function pointer, and the duplicate code can be
consolidated.

static int sev_do_init_locked(int cmd, void *data, int *error)
{
if (sev_es_tmr) {
/*
* Do not include the encryption mask on the physical
* address of the TMR (firmware should clear it anyway).
*/
data.flags |= SEV_INIT_FLAGS_SEV_ES;
data.tmr_address = __pa(sev_es_tmr);
data.tmr_len = SEV_ES_TMR_SIZE;
}
return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
}

static int __sev_init_locked(int *error)
{
struct sev_data_init data;

memset(&data, 0, sizeof(data));
return sev_do_init_locked(cmd, &data, error);
}

static int __sev_init_ex_locked(int *error)
{
struct sev_data_init_ex data;

memset(&data, 0, sizeof(data));
data.length = sizeof(data);
data.nv_address = __psp_pa(sev_init_ex_nv_address);
data.nv_len = NV_LENGTH;
return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
}

> + rc = init_function(error);
> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> /*
> * INIT command returned an integrity check failure
> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> * failed and persistent state has been erased.
> * Retrying INIT command here should succeed.
> */
> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> + dev_notice(sev->dev, "SEV: retrying INIT command");
> + rc = init_function(error);

The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
only writes back to the file if a relevant command was successful, which means
that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
with the same garbage data.

IMO, the behavior should be to read the file on load and then use the kernel buffer
without ever reloading (unless this is built as a module and is unloaded and reloaded).
The writeback then becomes opportunistic in the sense that if it fails for some reason,
the kernel's internal state isn't blasted away.

> }
>
> if (rc)
> @@ -303,7 +440,7 @@ static int __sev_platform_init_locked(int *error)
>
> dev_dbg(sev->dev, "SEV firmware initialized\n");
>
> - return rc;
> + return 0;
> }
>
> int sev_platform_init(int *error)
> @@ -1057,6 +1194,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
> get_order(SEV_ES_TMR_SIZE));
> sev_es_tmr = NULL;
> }
> +
> + if (sev_init_ex_nv_address) {

It really seems like the the teardown path should be paranoid and do a final
writeback of the kernel's buffer.

> + free_pages((unsigned long)sev_init_ex_nv_address,
> + get_order(NV_LENGTH));
> + sev_init_ex_nv_address = NULL;
> + }
> }
>
> void sev_dev_destroy(struct psp_device *psp)
> @@ -1101,6 +1244,18 @@ void sev_pci_init(void)
> sev_update_firmware(sev->dev) == 0)
> sev_get_api_version();
>
> + /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> + * instead of INIT.
> + */
> + if (init_ex_path) {
> + sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
> + if (!sev_init_ex_nv_address) {
> + dev_err(sev->dev,
> + "SEV: INIT_EX NV memory allocation failed\n");
> + goto err;

Why does this nullify psp_master->sev_data but later failures do not?

> + }
> + }
> +
> /* Obtain the TMR memory area for SEV-ES use */
> sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> if (!sev_es_tmr)
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index d48a7192e881..1595088c428b 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -52,6 +52,7 @@ enum sev_cmd {
> SEV_CMD_DF_FLUSH = 0x00A,
> SEV_CMD_DOWNLOAD_FIRMWARE = 0x00B,
> SEV_CMD_GET_ID = 0x00C,
> + SEV_CMD_INIT_EX = 0x00D,
>
> /* Guest commands */
> SEV_CMD_DECOMMISSION = 0x020,
> @@ -102,6 +103,26 @@ struct sev_data_init {
> u32 tmr_len; /* In */
> } __packed;
>
> +/**
> + * struct sev_data_init_ex - INIT_EX command parameters
> + *
> + * @length: len of the command buffer read by the PSP
> + * @flags: processing flags
> + * @tmr_address: system physical address used for SEV-ES
> + * @tmr_len: len of tmr_address
> + * @nv_address: system physical address used for PSP NV storage
> + * @nv_len: len of nv_address
> + */
> +struct sev_data_init_ex {
> + u32 length; /* In */
> + u32 flags; /* In */
> + u64 tmr_address; /* In */
> + u32 tmr_len; /* In */
> + u32 reserved; /* In */
> + u64 nv_address; /* In/Out */
> + u32 nv_len; /* In */
> +} __packed;
> +
> #define SEV_INIT_FLAGS_SEV_ES 0x01
>
> /**
> --
> 2.33.1.1089.g2158813163f-goog
>

2021-11-09 22:38:38

by Sean Christopherson

Subject: Re: [PATCH V3 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

On Tue, Nov 09, 2021, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson <[email protected]> wrote:
> >
> > On Tue, Nov 02, 2021, Peter Gonda wrote:
> > > This change moves the data corrupted retry of SEV_INIT into the
> >
> > Use imperative mood.
>
> Will do for next revision
>
> >
> > > __sev_platform_init_locked() function. This is for upcoming INIT_EX
> > > support as well as helping direct callers of
> > > __sev_platform_init_locked() which currently do not support the
> > > retry.
> > >
> > > Signed-off-by: Peter Gonda <[email protected]>
> > > Reviewed-by: Marc Orr <[email protected]>
> > > Acked-by: David Rientjes <[email protected]>
> > > Acked-by: Tom Lendacky <[email protected]>
> > > Cc: Tom Lendacky <[email protected]>
> > > Cc: Brijesh Singh <[email protected]>
> > > Cc: Marc Orr <[email protected]>
> > > Cc: Joerg Roedel <[email protected]>
> > > Cc: Herbert Xu <[email protected]>
> > > Cc: David Rientjes <[email protected]>
> > > Cc: John Allen <[email protected]>
> > > Cc: "David S. Miller" <[email protected]>
> > > Cc: Paolo Bonzini <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > ---
> > > drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
> > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > > index ec89a82ba267..e4bc833949a0 100644
> > > --- a/drivers/crypto/ccp/sev-dev.c
> > > +++ b/drivers/crypto/ccp/sev-dev.c
> > > @@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
> > > }
> > >
> > > rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > + if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >
> > There are no guarantees that @error is non-NULL as this is reachable via an
> > exported function, sev_platform_init(). Which ties in with my complaints in the
> > previous patch that the API is a bit of a mess.
>
> That seems like a bug from the caller right? Is it typical that we
> sanity-check the caller in these instances?

sev-dev.c needs to make up its mind. __sev_do_cmd_locked() very clearly allows
a NULL @error, ergo all of the wrappers for sev_do_cmd() support a NULL @error.

> For example the same comment could be made here:
> https://elixir.bootlin.com/linux/latest/source/drivers/crypto/ccp/sev-dev.c#L336
>
> ```
> static int sev_get_platform_state(int *state, int *error)
> {
> struct sev_user_data_status data;
> int rc;
>
> rc = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, error);
> if (rc)
> return rc;
>
> *state = data.state; <--- State could be null.

No, because this is an internal helper and all call sites can be easily audited.

> return rc;
> }
> ```
>
> Example outside of this driver:
> https://elixir.bootlin.com/linux/v5.15.1/source/arch/x86/kvm/x86.c#L468
>
> ```
> int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> {
> enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
> enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); <---
> msr_info could be null here
> u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
> (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
>
> if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
> return 1;
> if (!msr_info->host_initiated) {
> if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
> return 1;
> if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
> return 1;
> }
>
> kvm_lapic_set_base(vcpu, msr_info->data);
> kvm_recalculate_apic_map(vcpu->kvm);
> return 0;
> }
> EXPORT_SYMBOL_GPL(kvm_set_apic_base);
> ```

The difference is that KVM has consistent expecations for a set of functions,
whereas sev-dev.c does not. Yes, KVM will explode if @msr_info is NULL, and
there are undoubtedly a bajillion flows in the kernel that would do the same,
but unlike the functions declared in include/linux/psp-sev.h() the requirements
on the caller are fairly obvious. E.g. why should this be illegal from a caller's
perspective?

sev_platform_init(NULL);
sev_platform_status(&status, NULL);

2021-11-09 22:43:23

by Peter Gonda

Subject: Re: [PATCH V3 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

On Tue, Nov 9, 2021 at 10:31 AM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 9:31 AM Sean Christopherson <[email protected]> wrote:
> > >
> > > On Tue, Nov 02, 2021, Peter Gonda wrote:
> > > > This change moves the data corrupted retry of SEV_INIT into the
> > >
> > > Use imperative mood.
> >
> > Will do for next revision
> >
> > >
> > > > __sev_platform_init_locked() function. This is for upcoming INIT_EX
> > > > support as well as helping direct callers of
> > > > __sev_platform_init_locked() which currently do not support the
> > > > retry.
> > > >
> > > > Signed-off-by: Peter Gonda <[email protected]>
> > > > Reviewed-by: Marc Orr <[email protected]>
> > > > Acked-by: David Rientjes <[email protected]>
> > > > Acked-by: Tom Lendacky <[email protected]>
> > > > Cc: Tom Lendacky <[email protected]>
> > > > Cc: Brijesh Singh <[email protected]>
> > > > Cc: Marc Orr <[email protected]>
> > > > Cc: Joerg Roedel <[email protected]>
> > > > Cc: Herbert Xu <[email protected]>
> > > > Cc: David Rientjes <[email protected]>
> > > > Cc: John Allen <[email protected]>
> > > > Cc: "David S. Miller" <[email protected]>
> > > > Cc: Paolo Bonzini <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > ---
> > > > drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
> > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > > > index ec89a82ba267..e4bc833949a0 100644
> > > > --- a/drivers/crypto/ccp/sev-dev.c
> > > > +++ b/drivers/crypto/ccp/sev-dev.c
> > > > @@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
> > > > }
> > > >
> > > > rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > + if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > >
> > > There are no guarantees that @error is non-NULL as this is reachable via an
> > > exported function, sev_platform_init(). Which ties in with my complaints in the
> > > previous patch that the API is a bit of a mess.
> >
> > That seems like a bug from the caller right? Is it typical that we
> > sanity-check the caller in these instances?
>
> sev-dev.c needs to make up its mind. __sev_do_cmd_locked() very clearly allows
> a NULL @error, ergo all of the wrappers for sev_do_cmd() support a NULL @error.
>
> > For example the same comment could be made here:
> > https://elixir.bootlin.com/linux/latest/source/drivers/crypto/ccp/sev-dev.c#L336
> >
> > ```
> > static int sev_get_platform_state(int *state, int *error)
> > {
> > struct sev_user_data_status data;
> > int rc;
> >
> > rc = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, error);
> > if (rc)
> > return rc;
> >
> > *state = data.state; <--- State could be null.
>
> No, because this is an internal helper and all call sites can be easily audited.
>
> > return rc;
> > }
> > ```
> >
> > Example outside of this driver:
> > https://elixir.bootlin.com/linux/v5.15.1/source/arch/x86/kvm/x86.c#L468
> >
> > ```
> > int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > {
> > enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
> > enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); <---
> > msr_info could be null here
> > u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
> > (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
> >
> > if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
> > return 1;
> > if (!msr_info->host_initiated) {
> > if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
> > return 1;
> > if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
> > return 1;
> > }
> >
> > kvm_lapic_set_base(vcpu, msr_info->data);
> > kvm_recalculate_apic_map(vcpu->kvm);
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(kvm_set_apic_base);
> > ```
>
> The difference is that KVM has consistent expecations for a set of functions,
> whereas sev-dev.c does not. Yes, KVM will explode if @msr_info is NULL, and
> there are undoubtedly a bajillion flows in the kernel that would do the same,
> but unlike the functions declared in include/linux/psp-sev.h() the requirements
> on the caller are fairly obvious. E.g. why should this be illegal from a caller's
> perspective?
>
> sev_platform_init(NULL);
> sev_platform_status(&status, NULL);

Ack. I'll store a intermediate error in __sev_platform_init_locked and
export to @error if its not null.

2021-11-09 22:45:18

by Tom Lendacky

Subject: Re: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init

On 11/9/21 10:46 AM, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <[email protected]> wrote:
>> On Tue, Nov 02, 2021, Peter Gonda wrote:

...

>>
>> SEV: failed to INIT error 0, rc -16
>>
>> which a bit head scratching without looking at the code. AFAICT, the PSP return
>> codes aren't intrinsically hex, so printing error as a signed demical and thus
>>
>> SEV: failed to INIT error -1, rc -16
>>
>> would be less confusing.
>>
>> And IMO requiring the caller to initialize error is will be neverending game of
>> whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure,
>> and literally every function exposed via include/linux/psp-sev.h has this same
>> issue. Case in point, the retry path fails to re-initialize "error" and will
>> display stale information if the second sev_platform_init() fails without reaching
>> the PSP.
>
> OK I can update __sev_platform_init_locked() to set error to -1. That
> seems pretty reasonable. Tom, is that OK with you?

Yup, I'm ok with using -1.

>

...

>
> These comments seem fine to me. But I'll refrain from updating
> anything here since this seems out-of-scope of this series. Happy to
> discuss further and work on that if Tom is interested in those
> refactors too.

That's one of those things we've wanted to get around to improving but
just haven't had the time. So, yes, if you wish to refactor the 'error'
related area, that would be great.

Thanks,
Tom

>

2021-11-09 23:03:52

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

.

On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Nov 02, 2021, Peter Gonda wrote:
> > @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
> > module_param(psp_probe_timeout, int, 0644);
> > MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
> >
> > +static char *init_ex_path;
> > +module_param(init_ex_path, charp, 0660);
>
> Why is this writable after the module loads? At best, it seems like it will give
> userspace an easy way to shoot itself in the foot, at worst it will lead to a
> TOCTOU bug in the kernel.

Will update.

>
> > +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
> > +
> > 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 */
> > @@ -58,6 +63,14 @@ static int psp_timeout;
> > #define SEV_ES_TMR_SIZE (1024 * 1024)
> > static void *sev_es_tmr;
> >
> > +/* INIT_EX NV Storage:
> > + * The NV Storage is a 32Kb area and must be 4Kb page aligned. Use the page
> > + * allocator to allocate the memory, which will return aligned memory for the
> > + * specified allocation order.
> > + */
> > +#define NV_LENGTH (32 * 1024)
> > +static void *sev_init_ex_nv_address;
>
> The "address" part is redundant and potentially confusing, e.g. one might expect
> it to contain a physical address.
>
> And the "NV" part is kind of a lie, this isn't in non-volatile memory (it's a
> kernel buffer) and it's obviously volatile in the sense that it can be changed
> by the PSP. I get that from the PSP's perspective it's _intended_ to be NV
> storage, but (a) this does not point at NV storage and (b) there is no guarantee
> that the path provided by userspace points at NV storage.
>
> Maybe "sev_init_ex_buffer" or something along those lines?
>
> > static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > @@ -107,6 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
> > {
> > switch (cmd) {
> > case SEV_CMD_INIT: return sizeof(struct sev_data_init);
> > + case SEV_CMD_INIT_EX: return sizeof(struct sev_data_init_ex);
> > case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_user_data_status);
> > case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr);
> > case SEV_CMD_PEK_CERT_IMPORT: return sizeof(struct sev_data_pek_cert_import);
> > @@ -152,6 +166,87 @@ static void *sev_fw_alloc(unsigned long len)
> > return page_address(page);
> > }
> >
> > +static int sev_read_nv_memory(void)
>
> Similar to above, this is reading from a file that's backed by who knows what,
> i.e. it may or may not be reading NV memory. And if it's going to implicitly
> read into the buffer, that should be reflected in the name.
>
> > +{
> > + struct file *fp;
> > + ssize_t nread;
> > +
> > + if (!sev_init_ex_nv_address)
> > + return -EOPNOTSUPP;
> > +
> > + fp = filp_open(init_ex_path, O_RDONLY, 0);
> > + if (IS_ERR(fp)) {
> > + int ret = PTR_ERR(fp);
> > +
> > + dev_err(psp_master->dev,
> > + "SEV: could not open %s for read, error %d\n",
> > + init_ex_path, ret);
> > + return ret;
> > + }
> > +
> > + nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
> > + dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> > + filp_close(fp, NULL);
> > +
> > + return 0;
> > +}
> > +
> > +static void sev_write_nv_memory(void)
>
> Same comments here.
>
> > +{
> > + struct sev_device *sev = psp_master->sev_data;
> > + struct file *fp;
> > + loff_t offset = 0;
> > + ssize_t nwrite;
> > +
> > + if (!sev_init_ex_nv_address)
> > + return;
> > +
> > + fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> > + if (IS_ERR(fp)) {
> > + dev_err(sev->dev,
> > + "SEV: could not open file for write, error %d\n",
> > + PTR_ERR(fp));
> > + return;
> > + }
> > +
> > + nwrite = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> > + vfs_fsync(fp, 0);
> > + filp_close(fp, NULL);
> > +
> > + if (nwrite != NV_LENGTH) {
> > + dev_err(sev->dev,
> > + "SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
> > + NV_LENGTH, nwrite);
> > + return;
> > + }
> > +
> > + dev_dbg(sev->dev, "SEV: write successful to NV file\n");
> > +}
> > +
> > +static void sev_write_nv_memory_if_required(int cmd_id)
>
> And here.

I'll update the naming.

>
> > +{
> > + if (!sev_init_ex_nv_address)
> > + return;
> > +
> > + /*
> > + * Only a few platform commands modify the SPI/NV area, but none of the
> > + * non-platform commands do. Only INIT(_EX), PLATFORM_RESET, PEK_GEN,
> > + * PEK_CERT_IMPORT, and PDH_GEN do.
> > + */
> > + switch (cmd_id) {
> > + case SEV_CMD_FACTORY_RESET:
> > + case SEV_CMD_INIT_EX:
> > + case SEV_CMD_PDH_GEN:
> > + case SEV_CMD_PEK_CERT_IMPORT:
> > + case SEV_CMD_PEK_GEN:
> > + break;
> > + default:
> > + return;
> > + };
> > +
> > + sev_write_nv_memory();
> > +}
> > +
> > static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > {
> > struct psp_device *psp = psp_master;
> > @@ -221,6 +316,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
> > cmd, reg & PSP_CMDRESP_ERR_MASK);
> > ret = -EIO;
> > + } else {
> > + sev_write_nv_memory_if_required(cmd);
> > }
> >
> > print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> > @@ -247,22 +344,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
> > return rc;
> > }
> >
> > -static int __sev_platform_init_locked(int *error)
> > +static int __sev_init_locked(int *error)
> > {
> > - struct psp_device *psp = psp_master;
> > struct sev_data_init data;
> > - struct sev_device *sev;
> > - int rc = 0;
> >
> > - if (!psp || !psp->sev_data)
> > - return -ENODEV;
> > + memset(&data, 0, sizeof(data));
> > + if (sev_es_tmr) {
> > + u64 tmr_pa;
> >
> > - sev = psp->sev_data;
> > + /*
> > + * Do not include the encryption mask on the physical
> > + * address of the TMR (firmware should clear it anyway).
> > + */
> > + tmr_pa = __pa(sev_es_tmr);
>
> I realize this is copy-pasted from existing code, by why bother with an intermediate
> tmr_pa? Just set data.tmr_address directly.

I can fix that up in the next revison.

>
> >
> > - if (sev->state == SEV_STATE_INIT)
> > - return 0;
> > + data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > + data.tmr_address = tmr_pa;
> > + data.tmr_len = SEV_ES_TMR_SIZE;
> > + }
> > +
> > + return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > +}
> > +
> > +static int __sev_init_ex_locked(int *error)
> > +{
> > + struct sev_data_init_ex data;
> > + int ret;
> >
> > memset(&data, 0, sizeof(data));
> > + data.length = sizeof(data);
> > + data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > + data.nv_len = NV_LENGTH;
> > +
> > + ret = sev_read_nv_memory();
>
> Why defer reading the file until INIT_EX? Why not read the data when the path
> is first verified and consumed? More comments below in the retry path.
>
> And what happens when sev_platform_init() is reached through a path other than
> sev_pci_init()? I guess that's a question for the existing code since sev_es_tmr
> will be NULL unless sev_platform_init() routes through sev_pci_init(). So either
> the sev_platform_init() call from KVM's sev_guest_init() is pointless, or there's
> potential for some truly bizarre and confusing behavior.

Lets consolidate this discussion below.

>
> > + if (ret)
> > + return ret;
> > +
> > if (sev_es_tmr) {
> > u64 tmr_pa;
> >
> > @@ -277,7 +394,27 @@ static int __sev_platform_init_locked(int *error)
> > data.tmr_len = SEV_ES_TMR_SIZE;
> > }
> >
> > - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > + return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
> > +}
> > +
> > +static int __sev_platform_init_locked(int *error)
> > +{
> > + struct psp_device *psp = psp_master;
> > + struct sev_device *sev;
> > + int rc;
> > + int (*init_function)(int *error);
> > +
> > + if (!psp || !psp->sev_data)
> > + return -ENODEV;
> > +
> > + sev = psp->sev_data;
> > +
> > + if (sev->state == SEV_STATE_INIT)
> > + return 0;
> > +
> > + init_function = sev_init_ex_nv_address ? __sev_init_ex_locked :
> > + __sev_init_locked;
>
> There's no need for this to be a function pointer, and the duplicate code can be
> consolidated.
>
> static int sev_do_init_locked(int cmd, void *data, int *error)
> {
> if (sev_es_tmr) {
> /*
> * Do not include the encryption mask on the physical
> * address of the TMR (firmware should clear it anyway).
> */
> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> data.tmr_address = __pa(sev_es_tmr);
> data.tmr_len = SEV_ES_TMR_SIZE;
> }
> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> }
>
> static int __sev_init_locked(int *error)
> {
> struct sev_data_init data;
>
> memset(&data, 0, sizeof(data));
> return sev_do_init_locked(cmd, &data, error);
> }
>
> static int __sev_init_ex_locked(int *error)
> {
> struct sev_data_init_ex data;
>
> memset(&data, 0, sizeof(data));
> data.length = sizeof(data);
> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> data.nv_len = NV_LENGTH;
> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> }

I am missing how this removes the duplication of the retry code,
parameter checking, and other error checking code.. With what you have
typed out I would assume I still need to function pointer between
__sev_init_ex_locked and __sev_init_locked. Can you please elaborate
here? Also is there some reason the function pointer is not
acceptable?

>
> > + rc = init_function(error);
> > if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > /*
> > * INIT command returned an integrity check failure
> > @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > * failed and persistent state has been erased.
> > * Retrying INIT command here should succeed.
> > */
> > - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > + dev_notice(sev->dev, "SEV: retrying INIT command");
> > + rc = init_function(error);
>
> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> only writes back to the file if a relevant command was successful, which means
> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> with the same garbage data.

Ack my mistake, that comment is stale. I will update it so its correct
for the INIT and INIT_EX flows.
>
> IMO, the behavior should be to read the file on load and then use the kernel buffer
> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> the kernel's internal state isn't blasted away.

One issue here is that the file read can fail on load so we use the
late retry to guarantee we can read the file. The other point seems
like preference. Users may wish to shutdown the PSP FW, load a new
file, and INIT_EX again with that new data. Why should we preclude
them from that functionality?

>
> > }
> >
> > if (rc)
> > @@ -303,7 +440,7 @@ static int __sev_platform_init_locked(int *error)
> >
> > dev_dbg(sev->dev, "SEV firmware initialized\n");
> >
> > - return rc;
> > + return 0;
> > }
> >
> > int sev_platform_init(int *error)
> > @@ -1057,6 +1194,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
> > get_order(SEV_ES_TMR_SIZE));
> > sev_es_tmr = NULL;
> > }
> > +
> > + if (sev_init_ex_nv_address) {
>
> It really seems like the the teardown path should be paranoid and do a final
> writeback of the kernel's buffer.

Tom asked me to remove that in the last revision.

>
> > + free_pages((unsigned long)sev_init_ex_nv_address,
> > + get_order(NV_LENGTH));
> > + sev_init_ex_nv_address = NULL;
> > + }
> > }
> >
> > void sev_dev_destroy(struct psp_device *psp)
> > @@ -1101,6 +1244,18 @@ void sev_pci_init(void)
> > sev_update_firmware(sev->dev) == 0)
> > sev_get_api_version();
> >
> > + /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> > + * instead of INIT.
> > + */
> > + if (init_ex_path) {
> > + sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
> > + if (!sev_init_ex_nv_address) {
> > + dev_err(sev->dev,
> > + "SEV: INIT_EX NV memory allocation failed\n");
> > + goto err;
>
> Why does this nullify psp_master->sev_data but later failures do not?

If the buffer cannot be allocated then we cannot INIT_EX the PSP so
the operation is completely stopped similar to the error condition
with having an acceptable PSP API version.

>
> > + }
> > + }
> > +
> > /* Obtain the TMR memory area for SEV-ES use */
> > sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> > if (!sev_es_tmr)
> > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> > index d48a7192e881..1595088c428b 100644
> > --- a/include/linux/psp-sev.h
> > +++ b/include/linux/psp-sev.h
> > @@ -52,6 +52,7 @@ enum sev_cmd {
> > SEV_CMD_DF_FLUSH = 0x00A,
> > SEV_CMD_DOWNLOAD_FIRMWARE = 0x00B,
> > SEV_CMD_GET_ID = 0x00C,
> > + SEV_CMD_INIT_EX = 0x00D,
> >
> > /* Guest commands */
> > SEV_CMD_DECOMMISSION = 0x020,
> > @@ -102,6 +103,26 @@ struct sev_data_init {
> > u32 tmr_len; /* In */
> > } __packed;
> >
> > +/**
> > + * struct sev_data_init_ex - INIT_EX command parameters
> > + *
> > + * @length: len of the command buffer read by the PSP
> > + * @flags: processing flags
> > + * @tmr_address: system physical address used for SEV-ES
> > + * @tmr_len: len of tmr_address
> > + * @nv_address: system physical address used for PSP NV storage
> > + * @nv_len: len of nv_address
> > + */
> > +struct sev_data_init_ex {
> > + u32 length; /* In */
> > + u32 flags; /* In */
> > + u64 tmr_address; /* In */
> > + u32 tmr_len; /* In */
> > + u32 reserved; /* In */
> > + u64 nv_address; /* In/Out */
> > + u32 nv_len; /* In */
> > +} __packed;
> > +
> > #define SEV_INIT_FLAGS_SEV_ES 0x01
> >
> > /**
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

2021-11-09 23:13:02

by Sean Christopherson

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Tue, Nov 09, 2021, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> > There's no need for this to be a function pointer, and the duplicate code can be
> > consolidated.
> >
> > static int sev_do_init_locked(int cmd, void *data, int *error)
> > {
> > if (sev_es_tmr) {
> > /*
> > * Do not include the encryption mask on the physical
> > * address of the TMR (firmware should clear it anyway).
> > */
> > data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > data.tmr_address = __pa(sev_es_tmr);
> > data.tmr_len = SEV_ES_TMR_SIZE;
> > }
> > return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > }
> >
> > static int __sev_init_locked(int *error)
> > {
> > struct sev_data_init data;
> >
> > memset(&data, 0, sizeof(data));
> > return sev_do_init_locked(cmd, &data, error);
> > }
> >
> > static int __sev_init_ex_locked(int *error)
> > {
> > struct sev_data_init_ex data;
> >
> > memset(&data, 0, sizeof(data));
> > data.length = sizeof(data);
> > data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > data.nv_len = NV_LENGTH;
> > return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > }
>
> I am missing how this removes the duplication of the retry code,
> parameter checking, and other error checking code.. With what you have
> typed out I would assume I still need to function pointer between
> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> here?

Hmm. Ah, I got distracted between the original thought, the realization that
the two commands used different structs, and typing up the above.

> Also is there some reason the function pointer is not acceptable?

It's not unacceptable, it would just be nice to avoid, assuming the alternative
is cleaner. But I don't think any alternative is cleaner, since as you pointed
out the above is a half-baked thought.

> > > + rc = init_function(error);
> > > if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > /*
> > > * INIT command returned an integrity check failure
> > > @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > > * failed and persistent state has been erased.
> > > * Retrying INIT command here should succeed.
> > > */
> > > - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > > - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > + dev_notice(sev->dev, "SEV: retrying INIT command");
> > > + rc = init_function(error);
> >
> > The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > only writes back to the file if a relevant command was successful, which means
> > that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > with the same garbage data.
>
> Ack my mistake, that comment is stale. I will update it so its correct
> for the INIT and INIT_EX flows.
> >
> > IMO, the behavior should be to read the file on load and then use the kernel buffer
> > without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > the kernel's internal state isn't blasted away.
>
> One issue here is that the file read can fail on load so we use the
> late retry to guarantee we can read the file.

But why continue loading if reading the file fails on load?

> The other point seems like preference. Users may wish to shutdown the PSP FW,
> load a new file, and INIT_EX again with that new data. Why should we preclude
> them from that functionality?

I don't think we should preclude that functionality, but it needs to be explicitly
tied to a userspace action, e.g. either on module load or on writing the param to
change the path. If the latter is allowed, then it needs to be denied if the PSP
is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
userspace will have a heck of a time even understanding what state has been used
to initialize the PSP.

2021-11-10 00:19:05

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
>
> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> > > There's no need for this to be a function pointer, and the duplicate code can be
> > > consolidated.
> > >
> > > static int sev_do_init_locked(int cmd, void *data, int *error)
> > > {
> > > if (sev_es_tmr) {
> > > /*
> > > * Do not include the encryption mask on the physical
> > > * address of the TMR (firmware should clear it anyway).
> > > */
> > > data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > data.tmr_address = __pa(sev_es_tmr);
> > > data.tmr_len = SEV_ES_TMR_SIZE;
> > > }
> > > return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > }
> > >
> > > static int __sev_init_locked(int *error)
> > > {
> > > struct sev_data_init data;
> > >
> > > memset(&data, 0, sizeof(data));
> > > return sev_do_init_locked(cmd, &data, error);
> > > }
> > >
> > > static int __sev_init_ex_locked(int *error)
> > > {
> > > struct sev_data_init_ex data;
> > >
> > > memset(&data, 0, sizeof(data));
> > > data.length = sizeof(data);
> > > data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > data.nv_len = NV_LENGTH;
> > > return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > }
> >
> > I am missing how this removes the duplication of the retry code,
> > parameter checking, and other error checking code.. With what you have
> > typed out I would assume I still need to function pointer between
> > __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > here?
>
> Hmm. Ah, I got distracted between the original thought, the realization that
> the two commands used different structs, and typing up the above.
>
> > Also is there some reason the function pointer is not acceptable?
>
> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> out the above is a half-baked thought.

OK I'll leave as is.

>
> > > > + rc = init_function(error);
> > > > if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > > /*
> > > > * INIT command returned an integrity check failure
> > > > @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > > > * failed and persistent state has been erased.
> > > > * Retrying INIT command here should succeed.
> > > > */
> > > > - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > > > - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > + dev_notice(sev->dev, "SEV: retrying INIT command");
> > > > + rc = init_function(error);
> > >
> > > The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > only writes back to the file if a relevant command was successful, which means
> > > that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > with the same garbage data.
> >
> > Ack my mistake, that comment is stale. I will update it so its correct
> > for the INIT and INIT_EX flows.
> > >
> > > IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > the kernel's internal state isn't blasted away.
> >
> > One issue here is that the file read can fail on load so we use the
> > late retry to guarantee we can read the file.
>
> But why continue loading if reading the file fails on load?
>
> > The other point seems like preference. Users may wish to shutdown the PSP FW,
> > load a new file, and INIT_EX again with that new data. Why should we preclude
> > them from that functionality?
>
> I don't think we should preclude that functionality, but it needs to be explicitly
> tied to a userspace action, e.g. either on module load or on writing the param to
> change the path. If the latter is allowed, then it needs to be denied if the PSP
> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> userspace will have a heck of a time even understanding what state has been used
> to initialize the PSP.

If this driver is builtin the filesystem will be unavailable during
__init. Using the existing retries already built into
sev_platform_init() also the file to be read once userspace is
running, meaning the file system is usable. As I tried to explain in
the commit message. We could remove the sev_platform_init call during
sev_pci_init since this only actually needs to be initialized when the
first command requiring it is issues (either reading some keys/certs
from the PSP or launching an SEV guest). Then userspace in both the
builtin and module usage would know running one of those commands
cause the file to be read for PSP usage. Tom any thoughts on this?

2021-11-10 00:29:50

by Brijesh Singh

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On 11/9/21 2:46 PM, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
>>
>> On Tue, Nov 09, 2021, Peter Gonda wrote:
>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
>>>> There's no need for this to be a function pointer, and the duplicate code can be
>>>> consolidated.
>>>>
>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
>>>> {
>>>> if (sev_es_tmr) {
>>>> /*
>>>> * Do not include the encryption mask on the physical
>>>> * address of the TMR (firmware should clear it anyway).
>>>> */
>>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>>> data.tmr_address = __pa(sev_es_tmr);
>>>> data.tmr_len = SEV_ES_TMR_SIZE;
>>>> }
>>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>> }
>>>>
>>>> static int __sev_init_locked(int *error)
>>>> {
>>>> struct sev_data_init data;
>>>>
>>>> memset(&data, 0, sizeof(data));
>>>> return sev_do_init_locked(cmd, &data, error);
>>>> }
>>>>
>>>> static int __sev_init_ex_locked(int *error)
>>>> {
>>>> struct sev_data_init_ex data;
>>>>
>>>> memset(&data, 0, sizeof(data));
>>>> data.length = sizeof(data);
>>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>>> data.nv_len = NV_LENGTH;
>>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
>>>> }
>>>
>>> I am missing how this removes the duplication of the retry code,
>>> parameter checking, and other error checking code.. With what you have
>>> typed out I would assume I still need to function pointer between
>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
>>> here?
>>
>> Hmm. Ah, I got distracted between the original thought, the realization that
>> the two commands used different structs, and typing up the above.
>>
>>> Also is there some reason the function pointer is not acceptable?
>>
>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
>> is cleaner. But I don't think any alternative is cleaner, since as you pointed
>> out the above is a half-baked thought.
>
> OK I'll leave as is.
>
>>
>>>>> + rc = init_function(error);
>>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>>> /*
>>>>> * INIT command returned an integrity check failure
>>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
>>>>> * failed and persistent state has been erased.
>>>>> * Retrying INIT command here should succeed.
>>>>> */
>>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
>>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
>>>>> + rc = init_function(error);
>>>>
>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
>>>> only writes back to the file if a relevant command was successful, which means
>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
>>>> with the same garbage data.
>>>
>>> Ack my mistake, that comment is stale. I will update it so its correct
>>> for the INIT and INIT_EX flows.
>>>>
>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
>>>> the kernel's internal state isn't blasted away.
>>>
>>> One issue here is that the file read can fail on load so we use the
>>> late retry to guarantee we can read the file.
>>
>> But why continue loading if reading the file fails on load?
>>
>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
>>> load a new file, and INIT_EX again with that new data. Why should we preclude
>>> them from that functionality?
>>
>> I don't think we should preclude that functionality, but it needs to be explicitly
>> tied to a userspace action, e.g. either on module load or on writing the param to
>> change the path. If the latter is allowed, then it needs to be denied if the PSP
>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
>> userspace will have a heck of a time even understanding what state has been used
>> to initialize the PSP.
>
> If this driver is builtin the filesystem will be unavailable during
> __init. Using the existing retries already built into
> sev_platform_init() also the file to be read once userspace is
> running, meaning the file system is usable. As I tried to explain in
> the commit message. We could remove the sev_platform_init call during
> sev_pci_init since this only actually needs to be initialized when the
> first command requiring it is issues (either reading some keys/certs
> from the PSP or launching an SEV guest). Then userspace in both the
> builtin and module usage would know running one of those commands
> cause the file to be read for PSP usage. Tom any thoughts on this?
>

One thing to note is that if we do the INIT on the first command then
the first guest launch will take a longer. The init command is not
cheap (especially with the SNP, it may take a longer because it has to
do all those RMP setup etc). IIRC, in my early SEV series in I was doing
the INIT during the first command execution and based on the
recommendation moved to do the init on probe.

Should we add a module param to control whether to do INIT on probe or
delay until the first command ?

-Brijesh

2021-11-10 15:33:25

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
>
>
>
> On 11/9/21 2:46 PM, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> >>
> >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> >>>> There's no need for this to be a function pointer, and the duplicate code can be
> >>>> consolidated.
> >>>>
> >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> >>>> {
> >>>> if (sev_es_tmr) {
> >>>> /*
> >>>> * Do not include the encryption mask on the physical
> >>>> * address of the TMR (firmware should clear it anyway).
> >>>> */
> >>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>>> data.tmr_address = __pa(sev_es_tmr);
> >>>> data.tmr_len = SEV_ES_TMR_SIZE;
> >>>> }
> >>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>> }
> >>>>
> >>>> static int __sev_init_locked(int *error)
> >>>> {
> >>>> struct sev_data_init data;
> >>>>
> >>>> memset(&data, 0, sizeof(data));
> >>>> return sev_do_init_locked(cmd, &data, error);
> >>>> }
> >>>>
> >>>> static int __sev_init_ex_locked(int *error)
> >>>> {
> >>>> struct sev_data_init_ex data;
> >>>>
> >>>> memset(&data, 0, sizeof(data));
> >>>> data.length = sizeof(data);
> >>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>>> data.nv_len = NV_LENGTH;
> >>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> >>>> }
> >>>
> >>> I am missing how this removes the duplication of the retry code,
> >>> parameter checking, and other error checking code.. With what you have
> >>> typed out I would assume I still need to function pointer between
> >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> >>> here?
> >>
> >> Hmm. Ah, I got distracted between the original thought, the realization that
> >> the two commands used different structs, and typing up the above.
> >>
> >>> Also is there some reason the function pointer is not acceptable?
> >>
> >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> >> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> >> out the above is a half-baked thought.
> >
> > OK I'll leave as is.
> >
> >>
> >>>>> + rc = init_function(error);
> >>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>>> /*
> >>>>> * INIT command returned an integrity check failure
> >>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> >>>>> * failed and persistent state has been erased.
> >>>>> * Retrying INIT command here should succeed.
> >>>>> */
> >>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> >>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> >>>>> + rc = init_function(error);
> >>>>
> >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> >>>> only writes back to the file if a relevant command was successful, which means
> >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> >>>> with the same garbage data.
> >>>
> >>> Ack my mistake, that comment is stale. I will update it so its correct
> >>> for the INIT and INIT_EX flows.
> >>>>
> >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> >>>> the kernel's internal state isn't blasted away.
> >>>
> >>> One issue here is that the file read can fail on load so we use the
> >>> late retry to guarantee we can read the file.
> >>
> >> But why continue loading if reading the file fails on load?
> >>
> >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> >>> them from that functionality?
> >>
> >> I don't think we should preclude that functionality, but it needs to be explicitly
> >> tied to a userspace action, e.g. either on module load or on writing the param to
> >> change the path. If the latter is allowed, then it needs to be denied if the PSP
> >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> >> userspace will have a heck of a time even understanding what state has been used
> >> to initialize the PSP.
> >
> > If this driver is builtin the filesystem will be unavailable during
> > __init. Using the existing retries already built into
> > sev_platform_init() also the file to be read once userspace is
> > running, meaning the file system is usable. As I tried to explain in
> > the commit message. We could remove the sev_platform_init call during
> > sev_pci_init since this only actually needs to be initialized when the
> > first command requiring it is issues (either reading some keys/certs
> > from the PSP or launching an SEV guest). Then userspace in both the
> > builtin and module usage would know running one of those commands
> > cause the file to be read for PSP usage. Tom any thoughts on this?
> >
>
> One thing to note is that if we do the INIT on the first command then
> the first guest launch will take a longer. The init command is not
> cheap (especially with the SNP, it may take a longer because it has to
> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> the INIT during the first command execution and based on the
> recommendation moved to do the init on probe.
>
> Should we add a module param to control whether to do INIT on probe or
> delay until the first command ?

Thats a good point Brijesh. I've only been testing this with SEV and
ES so haven't noticed that long setup time. I like the idea of a
module parameter to decide when to INIT, that should satisfy Sean's
concern that the user doesn't know when the INIT_EX file would be read
and that there is extra retry code (duplicated between sev_pci_init
and all the PSP commands). I'll get started on that.

>
> -Brijesh

2021-11-10 17:30:10

by Peter Gonda

Subject: Re: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init

On Tue, Nov 9, 2021 at 12:25 PM Tom Lendacky <[email protected]> wrote:
>
> On 11/9/21 10:46 AM, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <[email protected]> wrote:
> >> On Tue, Nov 02, 2021, Peter Gonda wrote:
>
> ...
>
> >>
> >> SEV: failed to INIT error 0, rc -16
> >>
> >> which a bit head scratching without looking at the code. AFAICT, the PSP return
> >> codes aren't intrinsically hex, so printing error as a signed demical and thus
> >>
> >> SEV: failed to INIT error -1, rc -16
> >>
> >> would be less confusing.
> >>
> >> And IMO requiring the caller to initialize error is will be neverending game of
> >> whack-a-mole. E.g. sev_ioctl() fails to set "error" in the userspace structure,
> >> and literally every function exposed via include/linux/psp-sev.h has this same
> >> issue. Case in point, the retry path fails to re-initialize "error" and will
> >> display stale information if the second sev_platform_init() fails without reaching
> >> the PSP.
> >
> > OK I can update __sev_platform_init_locked() to set error to -1. That
> > seems pretty reasonable. Tom, is that OK with you?
>
> Yup, I'm ok with using -1.
>
> >
>
> ...
>
> >
> > These comments seem fine to me. But I'll refrain from updating
> > anything here since this seems out-of-scope of this series. Happy to
> > discuss further and work on that if Tom is interested in those
> > refactors too.
>
> That's one of those things we've wanted to get around to improving but
> just haven't had the time. So, yes, if you wish to refactor the 'error'
> related area, that would be great.

OK so when I actually sat down to work on this. I realized this is
bigger than I thought. If we want to have error == -1 for all return
from psp-sev.h functions where the PSP isn't called yet there are a
lot of changes. For example if CONFIG_CRYPTO_DEV_SP_PSP is not defined
all these stubs need to be to set `*errror == -`, basically all these
functions need to be updated.

So to keep this series more targeted. I think I'll drop the error
here. And just have this patch print the rc value. If what I said
above seems reasonable I'll do those error refactors. Are people
envisioning something else for the error fixups?

>
> Thanks,
> Tom
>
> >

2021-11-11 14:10:45

by Tom Lendacky

Subject: Re: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init

On 11/10/21 11:29 AM, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 12:25 PM Tom Lendacky <[email protected]> wrote:
>> On 11/9/21 10:46 AM, Peter Gonda wrote:
>>> On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <[email protected]> wrote:
>>>> On Tue, Nov 02, 2021, Peter Gonda wrote:
>>

...

>>
>> That's one of those things we've wanted to get around to improving but
>> just haven't had the time. So, yes, if you wish to refactor the 'error'
>> related area, that would be great.
>
> OK so when I actually sat down to work on this. I realized this is
> bigger than I thought. If we want to have error == -1 for all return
> from psp-sev.h functions where the PSP isn't called yet there are a
> lot of changes. For example if CONFIG_CRYPTO_DEV_SP_PSP is not defined
> all these stubs need to be to set `*errror == -`, basically all these
> functions need to be updated.
>
> So to keep this series more targeted. I think I'll drop the error
> here. And just have this patch print the rc value. If what I said

In that case, I think you should keep the error value and initialize it to
0. That is consistent with the other paths. Then, if you take on the
fixups, it can be changed then.

> above seems reasonable I'll do those error refactors. Are people
> envisioning something else for the error fixups?

The main refactoring I wanted was to make sure the caller didn't have to
initialize the error variable. Whether to initialize it to 0 or -1 wasn't
part of my original thoughts. But I do like the -1 value because,
theoretically, we shouldn't get such a value back from the PSP. So if the
value printed is not -1, that is an indication that the PSP API was called
no matter the value of rc.

Thanks,
Tom

>
>>
>> Thanks,
>> Tom
>>
>>>

2021-11-12 16:55:30

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
>
> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
> >
> >
> >
> > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> > >>
> > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > >>>> consolidated.
> > >>>>
> > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > >>>> {
> > >>>> if (sev_es_tmr) {
> > >>>> /*
> > >>>> * Do not include the encryption mask on the physical
> > >>>> * address of the TMR (firmware should clear it anyway).
> > >>>> */
> > >>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > >>>> data.tmr_address = __pa(sev_es_tmr);
> > >>>> data.tmr_len = SEV_ES_TMR_SIZE;
> > >>>> }
> > >>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > >>>> }
> > >>>>
> > >>>> static int __sev_init_locked(int *error)
> > >>>> {
> > >>>> struct sev_data_init data;
> > >>>>
> > >>>> memset(&data, 0, sizeof(data));
> > >>>> return sev_do_init_locked(cmd, &data, error);
> > >>>> }
> > >>>>
> > >>>> static int __sev_init_ex_locked(int *error)
> > >>>> {
> > >>>> struct sev_data_init_ex data;
> > >>>>
> > >>>> memset(&data, 0, sizeof(data));
> > >>>> data.length = sizeof(data);
> > >>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > >>>> data.nv_len = NV_LENGTH;
> > >>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > >>>> }
> > >>>
> > >>> I am missing how this removes the duplication of the retry code,
> > >>> parameter checking, and other error checking code.. With what you have
> > >>> typed out I would assume I still need to function pointer between
> > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > >>> here?
> > >>
> > >> Hmm. Ah, I got distracted between the original thought, the realization that
> > >> the two commands used different structs, and typing up the above.
> > >>
> > >>> Also is there some reason the function pointer is not acceptable?
> > >>
> > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > >> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> > >> out the above is a half-baked thought.
> > >
> > > OK I'll leave as is.
> > >
> > >>
> > >>>>> + rc = init_function(error);
> > >>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > >>>>> /*
> > >>>>> * INIT command returned an integrity check failure
> > >>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > >>>>> * failed and persistent state has been erased.
> > >>>>> * Retrying INIT command here should succeed.
> > >>>>> */
> > >>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > >>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > >>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> > >>>>> + rc = init_function(error);
> > >>>>
> > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > >>>> only writes back to the file if a relevant command was successful, which means
> > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > >>>> with the same garbage data.
> > >>>
> > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > >>> for the INIT and INIT_EX flows.
> > >>>>
> > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > >>>> the kernel's internal state isn't blasted away.
> > >>>
> > >>> One issue here is that the file read can fail on load so we use the
> > >>> late retry to guarantee we can read the file.
> > >>
> > >> But why continue loading if reading the file fails on load?
> > >>
> > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > >>> them from that functionality?
> > >>
> > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > >> change the path. If the latter is allowed, then it needs to be denied if the PSP
> > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > >> userspace will have a heck of a time even understanding what state has been used
> > >> to initialize the PSP.
> > >
> > > If this driver is builtin the filesystem will be unavailable during
> > > __init. Using the existing retries already built into
> > > sev_platform_init() also the file to be read once userspace is
> > > running, meaning the file system is usable. As I tried to explain in
> > > the commit message. We could remove the sev_platform_init call during
> > > sev_pci_init since this only actually needs to be initialized when the
> > > first command requiring it is issues (either reading some keys/certs
> > > from the PSP or launching an SEV guest). Then userspace in both the
> > > builtin and module usage would know running one of those commands
> > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > >
> >
> > One thing to note is that if we do the INIT on the first command then
> > the first guest launch will take a longer. The init command is not
> > cheap (especially with the SNP, it may take a longer because it has to
> > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > the INIT during the first command execution and based on the
> > recommendation moved to do the init on probe.
> >
> > Should we add a module param to control whether to do INIT on probe or
> > delay until the first command ?
>
> Thats a good point Brijesh. I've only been testing this with SEV and
> ES so haven't noticed that long setup time. I like the idea of a
> module parameter to decide when to INIT, that should satisfy Sean's
> concern that the user doesn't know when the INIT_EX file would be read
> and that there is extra retry code (duplicated between sev_pci_init
> and all the PSP commands). I'll get started on that.

I need a little guidance on how to proceed with this. Should I have
the new module parameter 'psp_init_on_probe' just disable PSP init on
module init if false. Or should it also disable PSP init during
command flow if it's true?

I was thinking I should just have 'psp_init_on_probe' default to true,
and if false it stops the PSP init during sev_pci_init(). If I add the
second change that seems like it changes the ABI. Thoughts?

>
> >
> > -Brijesh

2021-11-12 17:46:53

by Marc Orr

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Fri, Nov 12, 2021 at 8:55 AM Peter Gonda <[email protected]> wrote:
>
> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
> >
> > On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
> > >
> > >
> > >
> > > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> > > >>
> > > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> > > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > > >>>> consolidated.
> > > >>>>
> > > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > > >>>> {
> > > >>>> if (sev_es_tmr) {
> > > >>>> /*
> > > >>>> * Do not include the encryption mask on the physical
> > > >>>> * address of the TMR (firmware should clear it anyway).
> > > >>>> */
> > > >>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > >>>> data.tmr_address = __pa(sev_es_tmr);
> > > >>>> data.tmr_len = SEV_ES_TMR_SIZE;
> > > >>>> }
> > > >>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > >>>> }
> > > >>>>
> > > >>>> static int __sev_init_locked(int *error)
> > > >>>> {
> > > >>>> struct sev_data_init data;
> > > >>>>
> > > >>>> memset(&data, 0, sizeof(data));
> > > >>>> return sev_do_init_locked(cmd, &data, error);
> > > >>>> }
> > > >>>>
> > > >>>> static int __sev_init_ex_locked(int *error)
> > > >>>> {
> > > >>>> struct sev_data_init_ex data;
> > > >>>>
> > > >>>> memset(&data, 0, sizeof(data));
> > > >>>> data.length = sizeof(data);
> > > >>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > >>>> data.nv_len = NV_LENGTH;
> > > >>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > >>>> }
> > > >>>
> > > >>> I am missing how this removes the duplication of the retry code,
> > > >>> parameter checking, and other error checking code.. With what you have
> > > >>> typed out I would assume I still need to function pointer between
> > > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > > >>> here?
> > > >>
> > > >> Hmm. Ah, I got distracted between the original thought, the realization that
> > > >> the two commands used different structs, and typing up the above.
> > > >>
> > > >>> Also is there some reason the function pointer is not acceptable?
> > > >>
> > > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > > >> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> > > >> out the above is a half-baked thought.
> > > >
> > > > OK I'll leave as is.
> > > >
> > > >>
> > > >>>>> + rc = init_function(error);
> > > >>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > >>>>> /*
> > > >>>>> * INIT command returned an integrity check failure
> > > >>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > > >>>>> * failed and persistent state has been erased.
> > > >>>>> * Retrying INIT command here should succeed.
> > > >>>>> */
> > > >>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > > >>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > >>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> > > >>>>> + rc = init_function(error);
> > > >>>>
> > > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > >>>> only writes back to the file if a relevant command was successful, which means
> > > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > >>>> with the same garbage data.
> > > >>>
> > > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > > >>> for the INIT and INIT_EX flows.
> > > >>>>
> > > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > >>>> the kernel's internal state isn't blasted away.
> > > >>>
> > > >>> One issue here is that the file read can fail on load so we use the
> > > >>> late retry to guarantee we can read the file.
> > > >>
> > > >> But why continue loading if reading the file fails on load?
> > > >>
> > > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > > >>> them from that functionality?
> > > >>
> > > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > > >> change the path. If the latter is allowed, then it needs to be denied if the PSP
> > > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > > >> userspace will have a heck of a time even understanding what state has been used
> > > >> to initialize the PSP.
> > > >
> > > > If this driver is builtin the filesystem will be unavailable during
> > > > __init. Using the existing retries already built into
> > > > sev_platform_init() also the file to be read once userspace is
> > > > running, meaning the file system is usable. As I tried to explain in
> > > > the commit message. We could remove the sev_platform_init call during
> > > > sev_pci_init since this only actually needs to be initialized when the
> > > > first command requiring it is issues (either reading some keys/certs
> > > > from the PSP or launching an SEV guest). Then userspace in both the
> > > > builtin and module usage would know running one of those commands
> > > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > > >
> > >
> > > One thing to note is that if we do the INIT on the first command then
> > > the first guest launch will take a longer. The init command is not
> > > cheap (especially with the SNP, it may take a longer because it has to
> > > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > > the INIT during the first command execution and based on the
> > > recommendation moved to do the init on probe.
> > >
> > > Should we add a module param to control whether to do INIT on probe or
> > > delay until the first command ?
> >
> > Thats a good point Brijesh. I've only been testing this with SEV and
> > ES so haven't noticed that long setup time. I like the idea of a
> > module parameter to decide when to INIT, that should satisfy Sean's
> > concern that the user doesn't know when the INIT_EX file would be read
> > and that there is extra retry code (duplicated between sev_pci_init
> > and all the PSP commands). I'll get started on that.
>
> I need a little guidance on how to proceed with this. Should I have
> the new module parameter 'psp_init_on_probe' just disable PSP init on
> module init if false. Or should it also disable PSP init during
> command flow if it's true?
>
> I was thinking I should just have 'psp_init_on_probe' default to true,
> and if false it stops the PSP init during sev_pci_init(). If I add the
> second change that seems like it changes the ABI. Thoughts?

What about doing the INIT when we load the KVM module? Does that
resolve all of these problems? By the time we load the KVM module, we
know that the file system is up, which is the original problem we were
trying to solve. And the KVM module is most likely loaded before we
run the first guest.

2021-11-12 17:49:30

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Fri, Nov 12, 2021 at 10:46 AM Marc Orr <[email protected]> wrote:
>
> On Fri, Nov 12, 2021 at 8:55 AM Peter Gonda <[email protected]> wrote:
> >
> > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
> > > >
> > > >
> > > >
> > > > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> > > > >>
> > > > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > > > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> > > > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > > > >>>> consolidated.
> > > > >>>>
> > > > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > > > >>>> {
> > > > >>>> if (sev_es_tmr) {
> > > > >>>> /*
> > > > >>>> * Do not include the encryption mask on the physical
> > > > >>>> * address of the TMR (firmware should clear it anyway).
> > > > >>>> */
> > > > >>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > > >>>> data.tmr_address = __pa(sev_es_tmr);
> > > > >>>> data.tmr_len = SEV_ES_TMR_SIZE;
> > > > >>>> }
> > > > >>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > >>>> }
> > > > >>>>
> > > > >>>> static int __sev_init_locked(int *error)
> > > > >>>> {
> > > > >>>> struct sev_data_init data;
> > > > >>>>
> > > > >>>> memset(&data, 0, sizeof(data));
> > > > >>>> return sev_do_init_locked(cmd, &data, error);
> > > > >>>> }
> > > > >>>>
> > > > >>>> static int __sev_init_ex_locked(int *error)
> > > > >>>> {
> > > > >>>> struct sev_data_init_ex data;
> > > > >>>>
> > > > >>>> memset(&data, 0, sizeof(data));
> > > > >>>> data.length = sizeof(data);
> > > > >>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > > >>>> data.nv_len = NV_LENGTH;
> > > > >>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > > >>>> }
> > > > >>>
> > > > >>> I am missing how this removes the duplication of the retry code,
> > > > >>> parameter checking, and other error checking code.. With what you have
> > > > >>> typed out I would assume I still need to function pointer between
> > > > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > > > >>> here?
> > > > >>
> > > > >> Hmm. Ah, I got distracted between the original thought, the realization that
> > > > >> the two commands used different structs, and typing up the above.
> > > > >>
> > > > >>> Also is there some reason the function pointer is not acceptable?
> > > > >>
> > > > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > > > >> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> > > > >> out the above is a half-baked thought.
> > > > >
> > > > > OK I'll leave as is.
> > > > >
> > > > >>
> > > > >>>>> + rc = init_function(error);
> > > > >>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > > >>>>> /*
> > > > >>>>> * INIT command returned an integrity check failure
> > > > >>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > > > >>>>> * failed and persistent state has been erased.
> > > > >>>>> * Retrying INIT command here should succeed.
> > > > >>>>> */
> > > > >>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > > > >>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > >>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> > > > >>>>> + rc = init_function(error);
> > > > >>>>
> > > > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > > >>>> only writes back to the file if a relevant command was successful, which means
> > > > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > > >>>> with the same garbage data.
> > > > >>>
> > > > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > > > >>> for the INIT and INIT_EX flows.
> > > > >>>>
> > > > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > > >>>> the kernel's internal state isn't blasted away.
> > > > >>>
> > > > >>> One issue here is that the file read can fail on load so we use the
> > > > >>> late retry to guarantee we can read the file.
> > > > >>
> > > > >> But why continue loading if reading the file fails on load?
> > > > >>
> > > > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > > > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > > > >>> them from that functionality?
> > > > >>
> > > > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > > > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > > > >> change the path. If the latter is allowed, then it needs to be denied if the PSP
> > > > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > > > >> userspace will have a heck of a time even understanding what state has been used
> > > > >> to initialize the PSP.
> > > > >
> > > > > If this driver is builtin the filesystem will be unavailable during
> > > > > __init. Using the existing retries already built into
> > > > > sev_platform_init() also the file to be read once userspace is
> > > > > running, meaning the file system is usable. As I tried to explain in
> > > > > the commit message. We could remove the sev_platform_init call during
> > > > > sev_pci_init since this only actually needs to be initialized when the
> > > > > first command requiring it is issues (either reading some keys/certs
> > > > > from the PSP or launching an SEV guest). Then userspace in both the
> > > > > builtin and module usage would know running one of those commands
> > > > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > > > >
> > > >
> > > > One thing to note is that if we do the INIT on the first command then
> > > > the first guest launch will take a longer. The init command is not
> > > > cheap (especially with the SNP, it may take a longer because it has to
> > > > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > > > the INIT during the first command execution and based on the
> > > > recommendation moved to do the init on probe.
> > > >
> > > > Should we add a module param to control whether to do INIT on probe or
> > > > delay until the first command ?
> > >
> > > Thats a good point Brijesh. I've only been testing this with SEV and
> > > ES so haven't noticed that long setup time. I like the idea of a
> > > module parameter to decide when to INIT, that should satisfy Sean's
> > > concern that the user doesn't know when the INIT_EX file would be read
> > > and that there is extra retry code (duplicated between sev_pci_init
> > > and all the PSP commands). I'll get started on that.
> >
> > I need a little guidance on how to proceed with this. Should I have
> > the new module parameter 'psp_init_on_probe' just disable PSP init on
> > module init if false. Or should it also disable PSP init during
> > command flow if it's true?
> >
> > I was thinking I should just have 'psp_init_on_probe' default to true,
> > and if false it stops the PSP init during sev_pci_init(). If I add the
> > second change that seems like it changes the ABI. Thoughts?
>
> What about doing the INIT when we load the KVM module? Does that
> resolve all of these problems? By the time we load the KVM module, we
> know that the file system is up, which is the original problem we were
> trying to solve. And the KVM module is most likely loaded before we
> run the first guest.

KVM can be compiled as Y as well right? Then KVM module init is still too early.

2021-11-12 18:28:16

by Marc Orr

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Fri, Nov 12, 2021 at 9:49 AM Peter Gonda <[email protected]> wrote:
>
> On Fri, Nov 12, 2021 at 10:46 AM Marc Orr <[email protected]> wrote:
> >
> > On Fri, Nov 12, 2021 at 8:55 AM Peter Gonda <[email protected]> wrote:
> > >
> > > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
> > > >
> > > > On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > > > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> > > > > >>
> > > > > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > > > > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> > > > > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > > > > >>>> consolidated.
> > > > > >>>>
> > > > > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > > > > >>>> {
> > > > > >>>> if (sev_es_tmr) {
> > > > > >>>> /*
> > > > > >>>> * Do not include the encryption mask on the physical
> > > > > >>>> * address of the TMR (firmware should clear it anyway).
> > > > > >>>> */
> > > > > >>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > > > >>>> data.tmr_address = __pa(sev_es_tmr);
> > > > > >>>> data.tmr_len = SEV_ES_TMR_SIZE;
> > > > > >>>> }
> > > > > >>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > > >>>> }
> > > > > >>>>
> > > > > >>>> static int __sev_init_locked(int *error)
> > > > > >>>> {
> > > > > >>>> struct sev_data_init data;
> > > > > >>>>
> > > > > >>>> memset(&data, 0, sizeof(data));
> > > > > >>>> return sev_do_init_locked(cmd, &data, error);
> > > > > >>>> }
> > > > > >>>>
> > > > > >>>> static int __sev_init_ex_locked(int *error)
> > > > > >>>> {
> > > > > >>>> struct sev_data_init_ex data;
> > > > > >>>>
> > > > > >>>> memset(&data, 0, sizeof(data));
> > > > > >>>> data.length = sizeof(data);
> > > > > >>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > > > >>>> data.nv_len = NV_LENGTH;
> > > > > >>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > > > >>>> }
> > > > > >>>
> > > > > >>> I am missing how this removes the duplication of the retry code,
> > > > > >>> parameter checking, and other error checking code.. With what you have
> > > > > >>> typed out I would assume I still need to function pointer between
> > > > > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > > > > >>> here?
> > > > > >>
> > > > > >> Hmm. Ah, I got distracted between the original thought, the realization that
> > > > > >> the two commands used different structs, and typing up the above.
> > > > > >>
> > > > > >>> Also is there some reason the function pointer is not acceptable?
> > > > > >>
> > > > > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > > > > >> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> > > > > >> out the above is a half-baked thought.
> > > > > >
> > > > > > OK I'll leave as is.
> > > > > >
> > > > > >>
> > > > > >>>>> + rc = init_function(error);
> > > > > >>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > > > >>>>> /*
> > > > > >>>>> * INIT command returned an integrity check failure
> > > > > >>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > > > > >>>>> * failed and persistent state has been erased.
> > > > > >>>>> * Retrying INIT command here should succeed.
> > > > > >>>>> */
> > > > > >>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> > > > > >>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > > >>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> > > > > >>>>> + rc = init_function(error);
> > > > > >>>>
> > > > > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > > > >>>> only writes back to the file if a relevant command was successful, which means
> > > > > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > > > >>>> with the same garbage data.
> > > > > >>>
> > > > > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > > > > >>> for the INIT and INIT_EX flows.
> > > > > >>>>
> > > > > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > > > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > > > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > > > >>>> the kernel's internal state isn't blasted away.
> > > > > >>>
> > > > > >>> One issue here is that the file read can fail on load so we use the
> > > > > >>> late retry to guarantee we can read the file.
> > > > > >>
> > > > > >> But why continue loading if reading the file fails on load?
> > > > > >>
> > > > > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > > > > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > > > > >>> them from that functionality?
> > > > > >>
> > > > > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > > > > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > > > > >> change the path. If the latter is allowed, then it needs to be denied if the PSP
> > > > > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > > > > >> userspace will have a heck of a time even understanding what state has been used
> > > > > >> to initialize the PSP.
> > > > > >
> > > > > > If this driver is builtin the filesystem will be unavailable during
> > > > > > __init. Using the existing retries already built into
> > > > > > sev_platform_init() also the file to be read once userspace is
> > > > > > running, meaning the file system is usable. As I tried to explain in
> > > > > > the commit message. We could remove the sev_platform_init call during
> > > > > > sev_pci_init since this only actually needs to be initialized when the
> > > > > > first command requiring it is issues (either reading some keys/certs
> > > > > > from the PSP or launching an SEV guest). Then userspace in both the
> > > > > > builtin and module usage would know running one of those commands
> > > > > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > > > > >
> > > > >
> > > > > One thing to note is that if we do the INIT on the first command then
> > > > > the first guest launch will take a longer. The init command is not
> > > > > cheap (especially with the SNP, it may take a longer because it has to
> > > > > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > > > > the INIT during the first command execution and based on the
> > > > > recommendation moved to do the init on probe.
> > > > >
> > > > > Should we add a module param to control whether to do INIT on probe or
> > > > > delay until the first command ?
> > > >
> > > > Thats a good point Brijesh. I've only been testing this with SEV and
> > > > ES so haven't noticed that long setup time. I like the idea of a
> > > > module parameter to decide when to INIT, that should satisfy Sean's
> > > > concern that the user doesn't know when the INIT_EX file would be read
> > > > and that there is extra retry code (duplicated between sev_pci_init
> > > > and all the PSP commands). I'll get started on that.
> > >
> > > I need a little guidance on how to proceed with this. Should I have
> > > the new module parameter 'psp_init_on_probe' just disable PSP init on
> > > module init if false. Or should it also disable PSP init during
> > > command flow if it's true?
> > >
> > > I was thinking I should just have 'psp_init_on_probe' default to true,
> > > and if false it stops the PSP init during sev_pci_init(). If I add the
> > > second change that seems like it changes the ABI. Thoughts?
> >
> > What about doing the INIT when we load the KVM module? Does that
> > resolve all of these problems? By the time we load the KVM module, we
> > know that the file system is up, which is the original problem we were
> > trying to solve. And the KVM module is most likely loaded before we
> > run the first guest.
>
> KVM can be compiled as Y as well right? Then KVM module init is still too early.

I think even with KVM built in, it's guaranteed to load after the file system:

* KVM is loaded using `module_init()` (e.g., kvm-amd `module_init()` [1]).
* `module_init()` is defined as `__initcall()` [2].
* `__initcall()` is defined as `device_initcall()` [3].
* Finally, looking at [3] and scrolling up a few lines,
`device_init_call()`'s appear to happen after the file system init
calls.

[1] https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/svm/svm.c#L4673
[2] https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L88
[3] https://elixir.bootlin.com/linux/latest/source/include/linux/init.h#L296

2021-11-12 23:39:51

by Brijesh Singh

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On 11/12/21 10:55 AM, Peter Gonda wrote:
> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
>> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
>>>
>>>
>>> On 11/9/21 2:46 PM, Peter Gonda wrote:
>>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
>>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
>>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
>>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
>>>>>>> consolidated.
>>>>>>>
>>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
>>>>>>> {
>>>>>>> if (sev_es_tmr) {
>>>>>>> /*
>>>>>>> * Do not include the encryption mask on the physical
>>>>>>> * address of the TMR (firmware should clear it anyway).
>>>>>>> */
>>>>>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>>>>>> data.tmr_address = __pa(sev_es_tmr);
>>>>>>> data.tmr_len = SEV_ES_TMR_SIZE;
>>>>>>> }
>>>>>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>>>> }
>>>>>>>
>>>>>>> static int __sev_init_locked(int *error)
>>>>>>> {
>>>>>>> struct sev_data_init data;
>>>>>>>
>>>>>>> memset(&data, 0, sizeof(data));
>>>>>>> return sev_do_init_locked(cmd, &data, error);
>>>>>>> }
>>>>>>>
>>>>>>> static int __sev_init_ex_locked(int *error)
>>>>>>> {
>>>>>>> struct sev_data_init_ex data;
>>>>>>>
>>>>>>> memset(&data, 0, sizeof(data));
>>>>>>> data.length = sizeof(data);
>>>>>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>>>>>> data.nv_len = NV_LENGTH;
>>>>>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
>>>>>>> }
>>>>>> I am missing how this removes the duplication of the retry code,
>>>>>> parameter checking, and other error checking code.. With what you have
>>>>>> typed out I would assume I still need to function pointer between
>>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
>>>>>> here?
>>>>> Hmm. Ah, I got distracted between the original thought, the realization that
>>>>> the two commands used different structs, and typing up the above.
>>>>>
>>>>>> Also is there some reason the function pointer is not acceptable?
>>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
>>>>> is cleaner. But I don't think any alternative is cleaner, since as you pointed
>>>>> out the above is a half-baked thought.
>>>> OK I'll leave as is.
>>>>
>>>>>>>> + rc = init_function(error);
>>>>>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>>>>>> /*
>>>>>>>> * INIT command returned an integrity check failure
>>>>>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
>>>>>>>> * failed and persistent state has been erased.
>>>>>>>> * Retrying INIT command here should succeed.
>>>>>>>> */
>>>>>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
>>>>>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
>>>>>>>> + rc = init_function(error);
>>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
>>>>>>> only writes back to the file if a relevant command was successful, which means
>>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
>>>>>>> with the same garbage data.
>>>>>> Ack my mistake, that comment is stale. I will update it so its correct
>>>>>> for the INIT and INIT_EX flows.
>>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
>>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
>>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
>>>>>>> the kernel's internal state isn't blasted away.
>>>>>> One issue here is that the file read can fail on load so we use the
>>>>>> late retry to guarantee we can read the file.
>>>>> But why continue loading if reading the file fails on load?
>>>>>
>>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
>>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
>>>>>> them from that functionality?
>>>>> I don't think we should preclude that functionality, but it needs to be explicitly
>>>>> tied to a userspace action, e.g. either on module load or on writing the param to
>>>>> change the path. If the latter is allowed, then it needs to be denied if the PSP
>>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
>>>>> userspace will have a heck of a time even understanding what state has been used
>>>>> to initialize the PSP.
>>>> If this driver is builtin the filesystem will be unavailable during
>>>> __init. Using the existing retries already built into
>>>> sev_platform_init() also the file to be read once userspace is
>>>> running, meaning the file system is usable. As I tried to explain in
>>>> the commit message. We could remove the sev_platform_init call during
>>>> sev_pci_init since this only actually needs to be initialized when the
>>>> first command requiring it is issues (either reading some keys/certs
>>>> from the PSP or launching an SEV guest). Then userspace in both the
>>>> builtin and module usage would know running one of those commands
>>>> cause the file to be read for PSP usage. Tom any thoughts on this?
>>>>
>>> One thing to note is that if we do the INIT on the first command then
>>> the first guest launch will take a longer. The init command is not
>>> cheap (especially with the SNP, it may take a longer because it has to
>>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
>>> the INIT during the first command execution and based on the
>>> recommendation moved to do the init on probe.
>>>
>>> Should we add a module param to control whether to do INIT on probe or
>>> delay until the first command ?
>> Thats a good point Brijesh. I've only been testing this with SEV and
>> ES so haven't noticed that long setup time. I like the idea of a
>> module parameter to decide when to INIT, that should satisfy Sean's
>> concern that the user doesn't know when the INIT_EX file would be read
>> and that there is extra retry code (duplicated between sev_pci_init
>> and all the PSP commands). I'll get started on that.
> I need a little guidance on how to proceed with this. Should I have
> the new module parameter 'psp_init_on_probe' just disable PSP init on
> module init if false. Or should it also disable PSP init during
> command flow if it's true?
>
> I was thinking I should just have 'psp_init_on_probe' default to true,
> and if false it stops the PSP init during sev_pci_init(). If I add the
> second change that seems like it changes the ABI. Thoughts?
>
Good point that a module params may break the ABI. How about if we add a
new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
implementation will be similar to the PLATFORM_RESET; it will shutdown
the firmware then call INIT_EX. A platform provisioning tool may use ioctl.

-Brijesh

2021-11-12 23:45:02

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <[email protected]> wrote:
>
>
> On 11/12/21 10:55 AM, Peter Gonda wrote:
> > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
> >> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
> >>>
> >>>
> >>> On 11/9/21 2:46 PM, Peter Gonda wrote:
> >>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> >>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
> >>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> >>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
> >>>>>>> consolidated.
> >>>>>>>
> >>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> >>>>>>> {
> >>>>>>> if (sev_es_tmr) {
> >>>>>>> /*
> >>>>>>> * Do not include the encryption mask on the physical
> >>>>>>> * address of the TMR (firmware should clear it anyway).
> >>>>>>> */
> >>>>>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>>>>>> data.tmr_address = __pa(sev_es_tmr);
> >>>>>>> data.tmr_len = SEV_ES_TMR_SIZE;
> >>>>>>> }
> >>>>>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>>>> }
> >>>>>>>
> >>>>>>> static int __sev_init_locked(int *error)
> >>>>>>> {
> >>>>>>> struct sev_data_init data;
> >>>>>>>
> >>>>>>> memset(&data, 0, sizeof(data));
> >>>>>>> return sev_do_init_locked(cmd, &data, error);
> >>>>>>> }
> >>>>>>>
> >>>>>>> static int __sev_init_ex_locked(int *error)
> >>>>>>> {
> >>>>>>> struct sev_data_init_ex data;
> >>>>>>>
> >>>>>>> memset(&data, 0, sizeof(data));
> >>>>>>> data.length = sizeof(data);
> >>>>>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>>>>>> data.nv_len = NV_LENGTH;
> >>>>>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> >>>>>>> }
> >>>>>> I am missing how this removes the duplication of the retry code,
> >>>>>> parameter checking, and other error checking code.. With what you have
> >>>>>> typed out I would assume I still need to function pointer between
> >>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> >>>>>> here?
> >>>>> Hmm. Ah, I got distracted between the original thought, the realization that
> >>>>> the two commands used different structs, and typing up the above.
> >>>>>
> >>>>>> Also is there some reason the function pointer is not acceptable?
> >>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> >>>>> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> >>>>> out the above is a half-baked thought.
> >>>> OK I'll leave as is.
> >>>>
> >>>>>>>> + rc = init_function(error);
> >>>>>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>>>>>> /*
> >>>>>>>> * INIT command returned an integrity check failure
> >>>>>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> >>>>>>>> * failed and persistent state has been erased.
> >>>>>>>> * Retrying INIT command here should succeed.
> >>>>>>>> */
> >>>>>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> >>>>>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> >>>>>>>> + rc = init_function(error);
> >>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> >>>>>>> only writes back to the file if a relevant command was successful, which means
> >>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> >>>>>>> with the same garbage data.
> >>>>>> Ack my mistake, that comment is stale. I will update it so its correct
> >>>>>> for the INIT and INIT_EX flows.
> >>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> >>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> >>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> >>>>>>> the kernel's internal state isn't blasted away.
> >>>>>> One issue here is that the file read can fail on load so we use the
> >>>>>> late retry to guarantee we can read the file.
> >>>>> But why continue loading if reading the file fails on load?
> >>>>>
> >>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> >>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
> >>>>>> them from that functionality?
> >>>>> I don't think we should preclude that functionality, but it needs to be explicitly
> >>>>> tied to a userspace action, e.g. either on module load or on writing the param to
> >>>>> change the path. If the latter is allowed, then it needs to be denied if the PSP
> >>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> >>>>> userspace will have a heck of a time even understanding what state has been used
> >>>>> to initialize the PSP.
> >>>> If this driver is builtin the filesystem will be unavailable during
> >>>> __init. Using the existing retries already built into
> >>>> sev_platform_init() also the file to be read once userspace is
> >>>> running, meaning the file system is usable. As I tried to explain in
> >>>> the commit message. We could remove the sev_platform_init call during
> >>>> sev_pci_init since this only actually needs to be initialized when the
> >>>> first command requiring it is issues (either reading some keys/certs
> >>>> from the PSP or launching an SEV guest). Then userspace in both the
> >>>> builtin and module usage would know running one of those commands
> >>>> cause the file to be read for PSP usage. Tom any thoughts on this?
> >>>>
> >>> One thing to note is that if we do the INIT on the first command then
> >>> the first guest launch will take a longer. The init command is not
> >>> cheap (especially with the SNP, it may take a longer because it has to
> >>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> >>> the INIT during the first command execution and based on the
> >>> recommendation moved to do the init on probe.
> >>>
> >>> Should we add a module param to control whether to do INIT on probe or
> >>> delay until the first command ?
> >> Thats a good point Brijesh. I've only been testing this with SEV and
> >> ES so haven't noticed that long setup time. I like the idea of a
> >> module parameter to decide when to INIT, that should satisfy Sean's
> >> concern that the user doesn't know when the INIT_EX file would be read
> >> and that there is extra retry code (duplicated between sev_pci_init
> >> and all the PSP commands). I'll get started on that.
> > I need a little guidance on how to proceed with this. Should I have
> > the new module parameter 'psp_init_on_probe' just disable PSP init on
> > module init if false. Or should it also disable PSP init during
> > command flow if it's true?
> >
> > I was thinking I should just have 'psp_init_on_probe' default to true,
> > and if false it stops the PSP init during sev_pci_init(). If I add the
> > second change that seems like it changes the ABI. Thoughts?
> >
> Good point that a module params may break the ABI. How about if we add a
> new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
> implementation will be similar to the PLATFORM_RESET; it will shutdown
> the firmware then call INIT_EX. A platform provisioning tool may use ioctl.

Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
to false but if users set it, we can skip that init attempt in
sev_pci_init(). The init attempts on all other commands that require
the INIT state would then provide users with INIT_EX functionality.
They would also know exactly when INIT or INIT_EX would be attempted
based on the parameter.

Otherwise a new ioctl sounds reasonable.

>
> -Brijesh
>

2021-11-12 23:50:55

by Brijesh Singh

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On 11/12/21 5:44 PM, Peter Gonda wrote:
> On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <[email protected]> wrote:
>>
>> On 11/12/21 10:55 AM, Peter Gonda wrote:
>>> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
>>>> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
>>>>>
>>>>> On 11/9/21 2:46 PM, Peter Gonda wrote:
>>>>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
>>>>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
>>>>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
>>>>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
>>>>>>>>> consolidated.
>>>>>>>>>
>>>>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
>>>>>>>>> {
>>>>>>>>> if (sev_es_tmr) {
>>>>>>>>> /*
>>>>>>>>> * Do not include the encryption mask on the physical
>>>>>>>>> * address of the TMR (firmware should clear it anyway).
>>>>>>>>> */
>>>>>>>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>>>>>>>> data.tmr_address = __pa(sev_es_tmr);
>>>>>>>>> data.tmr_len = SEV_ES_TMR_SIZE;
>>>>>>>>> }
>>>>>>>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static int __sev_init_locked(int *error)
>>>>>>>>> {
>>>>>>>>> struct sev_data_init data;
>>>>>>>>>
>>>>>>>>> memset(&data, 0, sizeof(data));
>>>>>>>>> return sev_do_init_locked(cmd, &data, error);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static int __sev_init_ex_locked(int *error)
>>>>>>>>> {
>>>>>>>>> struct sev_data_init_ex data;
>>>>>>>>>
>>>>>>>>> memset(&data, 0, sizeof(data));
>>>>>>>>> data.length = sizeof(data);
>>>>>>>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>>>>>>>> data.nv_len = NV_LENGTH;
>>>>>>>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
>>>>>>>>> }
>>>>>>>> I am missing how this removes the duplication of the retry code,
>>>>>>>> parameter checking, and other error checking code.. With what you have
>>>>>>>> typed out I would assume I still need to function pointer between
>>>>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
>>>>>>>> here?
>>>>>>> Hmm. Ah, I got distracted between the original thought, the realization that
>>>>>>> the two commands used different structs, and typing up the above.
>>>>>>>
>>>>>>>> Also is there some reason the function pointer is not acceptable?
>>>>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
>>>>>>> is cleaner. But I don't think any alternative is cleaner, since as you pointed
>>>>>>> out the above is a half-baked thought.
>>>>>> OK I'll leave as is.
>>>>>>
>>>>>>>>>> + rc = init_function(error);
>>>>>>>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>>>>>>>> /*
>>>>>>>>>> * INIT command returned an integrity check failure
>>>>>>>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
>>>>>>>>>> * failed and persistent state has been erased.
>>>>>>>>>> * Retrying INIT command here should succeed.
>>>>>>>>>> */
>>>>>>>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
>>>>>>>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>>>>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
>>>>>>>>>> + rc = init_function(error);
>>>>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
>>>>>>>>> only writes back to the file if a relevant command was successful, which means
>>>>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
>>>>>>>>> with the same garbage data.
>>>>>>>> Ack my mistake, that comment is stale. I will update it so its correct
>>>>>>>> for the INIT and INIT_EX flows.
>>>>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
>>>>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
>>>>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
>>>>>>>>> the kernel's internal state isn't blasted away.
>>>>>>>> One issue here is that the file read can fail on load so we use the
>>>>>>>> late retry to guarantee we can read the file.
>>>>>>> But why continue loading if reading the file fails on load?
>>>>>>>
>>>>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
>>>>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
>>>>>>>> them from that functionality?
>>>>>>> I don't think we should preclude that functionality, but it needs to be explicitly
>>>>>>> tied to a userspace action, e.g. either on module load or on writing the param to
>>>>>>> change the path. If the latter is allowed, then it needs to be denied if the PSP
>>>>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
>>>>>>> userspace will have a heck of a time even understanding what state has been used
>>>>>>> to initialize the PSP.
>>>>>> If this driver is builtin the filesystem will be unavailable during
>>>>>> __init. Using the existing retries already built into
>>>>>> sev_platform_init() also the file to be read once userspace is
>>>>>> running, meaning the file system is usable. As I tried to explain in
>>>>>> the commit message. We could remove the sev_platform_init call during
>>>>>> sev_pci_init since this only actually needs to be initialized when the
>>>>>> first command requiring it is issues (either reading some keys/certs
>>>>>> from the PSP or launching an SEV guest). Then userspace in both the
>>>>>> builtin and module usage would know running one of those commands
>>>>>> cause the file to be read for PSP usage. Tom any thoughts on this?
>>>>>>
>>>>> One thing to note is that if we do the INIT on the first command then
>>>>> the first guest launch will take a longer. The init command is not
>>>>> cheap (especially with the SNP, it may take a longer because it has to
>>>>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
>>>>> the INIT during the first command execution and based on the
>>>>> recommendation moved to do the init on probe.
>>>>>
>>>>> Should we add a module param to control whether to do INIT on probe or
>>>>> delay until the first command ?
>>>> Thats a good point Brijesh. I've only been testing this with SEV and
>>>> ES so haven't noticed that long setup time. I like the idea of a
>>>> module parameter to decide when to INIT, that should satisfy Sean's
>>>> concern that the user doesn't know when the INIT_EX file would be read
>>>> and that there is extra retry code (duplicated between sev_pci_init
>>>> and all the PSP commands). I'll get started on that.
>>> I need a little guidance on how to proceed with this. Should I have
>>> the new module parameter 'psp_init_on_probe' just disable PSP init on
>>> module init if false. Or should it also disable PSP init during
>>> command flow if it's true?
>>>
>>> I was thinking I should just have 'psp_init_on_probe' default to true,
>>> and if false it stops the PSP init during sev_pci_init(). If I add the
>>> second change that seems like it changes the ABI. Thoughts?
>>>
>> Good point that a module params may break the ABI. How about if we add a
>> new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
>> implementation will be similar to the PLATFORM_RESET; it will shutdown
>> the firmware then call INIT_EX. A platform provisioning tool may use ioctl.
> Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
> to false but if users set it, we can skip that init attempt in
> sev_pci_init(). The init attempts on all other commands that require
> the INIT state would then provide users with INIT_EX functionality.
> They would also know exactly when INIT or INIT_EX would be attempted
> based on the parameter.

Yes, I think that option is also acceptable. Because we are requiring
the user to explicitly say that it does not want to INIT on boot.


>
> Otherwise a new ioctl sounds reasonable.
>> -Brijesh
>>

2021-11-16 01:43:07

by Peter Gonda

Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Fri, Nov 12, 2021 at 4:50 PM Brijesh Singh <[email protected]> wrote:
>
>
> On 11/12/21 5:44 PM, Peter Gonda wrote:
> > On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <[email protected]> wrote:
> >>
> >> On 11/12/21 10:55 AM, Peter Gonda wrote:
> >>> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[email protected]> wrote:
> >>>> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[email protected]> wrote:
> >>>>>
> >>>>> On 11/9/21 2:46 PM, Peter Gonda wrote:
> >>>>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[email protected]> wrote:
> >>>>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
> >>>>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[email protected]> wrote:
> >>>>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
> >>>>>>>>> consolidated.
> >>>>>>>>>
> >>>>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> >>>>>>>>> {
> >>>>>>>>> if (sev_es_tmr) {
> >>>>>>>>> /*
> >>>>>>>>> * Do not include the encryption mask on the physical
> >>>>>>>>> * address of the TMR (firmware should clear it anyway).
> >>>>>>>>> */
> >>>>>>>>> data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>>>>>>>> data.tmr_address = __pa(sev_es_tmr);
> >>>>>>>>> data.tmr_len = SEV_ES_TMR_SIZE;
> >>>>>>>>> }
> >>>>>>>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> static int __sev_init_locked(int *error)
> >>>>>>>>> {
> >>>>>>>>> struct sev_data_init data;
> >>>>>>>>>
> >>>>>>>>> memset(&data, 0, sizeof(data));
> >>>>>>>>> return sev_do_init_locked(cmd, &data, error);
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> static int __sev_init_ex_locked(int *error)
> >>>>>>>>> {
> >>>>>>>>> struct sev_data_init_ex data;
> >>>>>>>>>
> >>>>>>>>> memset(&data, 0, sizeof(data));
> >>>>>>>>> data.length = sizeof(data);
> >>>>>>>>> data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>>>>>>>> data.nv_len = NV_LENGTH;
> >>>>>>>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> >>>>>>>>> }
> >>>>>>>> I am missing how this removes the duplication of the retry code,
> >>>>>>>> parameter checking, and other error checking code.. With what you have
> >>>>>>>> typed out I would assume I still need to function pointer between
> >>>>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> >>>>>>>> here?
> >>>>>>> Hmm. Ah, I got distracted between the original thought, the realization that
> >>>>>>> the two commands used different structs, and typing up the above.
> >>>>>>>
> >>>>>>>> Also is there some reason the function pointer is not acceptable?
> >>>>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> >>>>>>> is cleaner. But I don't think any alternative is cleaner, since as you pointed
> >>>>>>> out the above is a half-baked thought.
> >>>>>> OK I'll leave as is.
> >>>>>>
> >>>>>>>>>> + rc = init_function(error);
> >>>>>>>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>>>>>>>> /*
> >>>>>>>>>> * INIT command returned an integrity check failure
> >>>>>>>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> >>>>>>>>>> * failed and persistent state has been erased.
> >>>>>>>>>> * Retrying INIT command here should succeed.
> >>>>>>>>>> */
> >>>>>>>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command");
> >>>>>>>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>>>>>>> + dev_notice(sev->dev, "SEV: retrying INIT command");
> >>>>>>>>>> + rc = init_function(error);
> >>>>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> >>>>>>>>> only writes back to the file if a relevant command was successful, which means
> >>>>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> >>>>>>>>> with the same garbage data.
> >>>>>>>> Ack my mistake, that comment is stale. I will update it so its correct
> >>>>>>>> for the INIT and INIT_EX flows.
> >>>>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> >>>>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> >>>>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> >>>>>>>>> the kernel's internal state isn't blasted away.
> >>>>>>>> One issue here is that the file read can fail on load so we use the
> >>>>>>>> late retry to guarantee we can read the file.
> >>>>>>> But why continue loading if reading the file fails on load?
> >>>>>>>
> >>>>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> >>>>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
> >>>>>>>> them from that functionality?
> >>>>>>> I don't think we should preclude that functionality, but it needs to be explicitly
> >>>>>>> tied to a userspace action, e.g. either on module load or on writing the param to
> >>>>>>> change the path. If the latter is allowed, then it needs to be denied if the PSP
> >>>>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> >>>>>>> userspace will have a heck of a time even understanding what state has been used
> >>>>>>> to initialize the PSP.
> >>>>>> If this driver is builtin the filesystem will be unavailable during
> >>>>>> __init. Using the existing retries already built into
> >>>>>> sev_platform_init() also the file to be read once userspace is
> >>>>>> running, meaning the file system is usable. As I tried to explain in
> >>>>>> the commit message. We could remove the sev_platform_init call during
> >>>>>> sev_pci_init since this only actually needs to be initialized when the
> >>>>>> first command requiring it is issues (either reading some keys/certs
> >>>>>> from the PSP or launching an SEV guest). Then userspace in both the
> >>>>>> builtin and module usage would know running one of those commands
> >>>>>> cause the file to be read for PSP usage. Tom any thoughts on this?
> >>>>>>
> >>>>> One thing to note is that if we do the INIT on the first command then
> >>>>> the first guest launch will take a longer. The init command is not
> >>>>> cheap (especially with the SNP, it may take a longer because it has to
> >>>>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> >>>>> the INIT during the first command execution and based on the
> >>>>> recommendation moved to do the init on probe.
> >>>>>
> >>>>> Should we add a module param to control whether to do INIT on probe or
> >>>>> delay until the first command ?
> >>>> Thats a good point Brijesh. I've only been testing this with SEV and
> >>>> ES so haven't noticed that long setup time. I like the idea of a
> >>>> module parameter to decide when to INIT, that should satisfy Sean's
> >>>> concern that the user doesn't know when the INIT_EX file would be read
> >>>> and that there is extra retry code (duplicated between sev_pci_init
> >>>> and all the PSP commands). I'll get started on that.
> >>> I need a little guidance on how to proceed with this. Should I have
> >>> the new module parameter 'psp_init_on_probe' just disable PSP init on
> >>> module init if false. Or should it also disable PSP init during
> >>> command flow if it's true?
> >>>
> >>> I was thinking I should just have 'psp_init_on_probe' default to true,
> >>> and if false it stops the PSP init during sev_pci_init(). If I add the
> >>> second change that seems like it changes the ABI. Thoughts?
> >>>
> >> Good point that a module params may break the ABI. How about if we add a
> >> new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
> >> implementation will be similar to the PLATFORM_RESET; it will shutdown
> >> the firmware then call INIT_EX. A platform provisioning tool may use ioctl.
> > Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
> > to false but if users set it, we can skip that init attempt in
> > sev_pci_init(). The init attempts on all other commands that require
> > the INIT state would then provide users with INIT_EX functionality.
> > They would also know exactly when INIT or INIT_EX would be attempted
> > based on the parameter.
>
> Yes, I think that option is also acceptable. Because we are requiring
> the user to explicitly say that it does not want to INIT on boot.

OK sent out a V4 with this mode param approach.

>
>
> >
> > Otherwise a new ioctl sounds reasonable.
> >> -Brijesh
> >>