Re: [PATCH v2] crypto: ccp: Sanitize sev_platform_init() error messages

From: Tom Lendacky
Date: Mon Jan 09 2023 - 09:34:18 EST

On 1/8/23 14:24, Jarkko Sakkinen wrote:
The following functions end up calling sev_platform_init() or
__sev_platform_init_locked():

* sev_guest_init()
* sev_ioctl_do_pek_csr
* sev_ioctl_do_pdh_export()
* sev_ioctl_do_pek_import()
* sev_ioctl_do_pek_pdh_gen()
* sev_pci_init()

However, only sev_pci_init() prints out the failed command error code, and
even there the error message does not specify, SEV which command failed.

event there, the error message does not specify which SEV command failed.


Address this by printing out the SEV command errors inside
__sev_platform_init_locked(), and differentiate between DF_FLUSH, INIT and
INIT_EX commands. As a side-effect, @error can be removed from the
parameter list.

This extra information is particularly useful if firmware loading and/or
initialization is going to be made more robust, e.g. by allowing firmware
loading to be postponed.

Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxxx>
---
v2:
* Address David Rientjes's feedback:
https://lore.kernel.org/all/6a16bbe4-4281-fb28-78c4-4ec44c8aa679@xxxxxxxxxx/
* Remove @error.
* Remove "SEV_" prefix: it is obvious from context so no need to make klog
line longer.
---
arch/x86/kvm/svm/sev.c | 2 +-
drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
include/linux/psp-sev.h | 4 +---
3 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index efaaef2b7ae1..42e6bd637896 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -254,7 +254,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
goto e_no_asid;
sev->asid = asid;
- ret = sev_platform_init(&argp->error);
+ ret = sev_platform_init();
if (ret)
goto e_free;
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 06fc7156c04f..3b66cb1495d4 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -440,7 +440,7 @@ static int __sev_init_ex_locked(int *error)
return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
}
-static int __sev_platform_init_locked(int *error)
+static int __sev_platform_init_locked(void)

Why is this being removed. Userspace may still want to see this. Some of the error pointers are from the ioctl() arguments...

{
struct psp_device *psp = psp_master;
struct sev_device *sev;
@@ -476,19 +476,21 @@ static int __sev_platform_init_locked(int *error)
dev_err(sev->dev, "SEV: retrying INIT command because of SECURE_DATA_INVALID error. Retrying once to reset PSP SEV state.");
rc = init_function(&psp_ret);
}
- if (error)
- *error = psp_ret;
-
- if (rc)
+ if (rc) {
+ dev_err(sev->dev, "SEV: %s failed error %#x",
+ sev_init_ex_buffer ? "CMD_INIT_EX" : "CMD_INIT", psp_ret);
return rc;
+ }
sev->state = SEV_STATE_INIT;
/* Prepare for first SEV guest launch after INIT */
wbinvd_on_all_cpus();
- rc = __sev_do_cmd_locked(SEV_CMD_DF_FLUSH, NULL, error);
- if (rc)
+ rc = __sev_do_cmd_locked(SEV_CMD_DF_FLUSH, NULL, &psp_ret);
+ if (rc) {
+ dev_err(sev->dev, "SEV: CMD_DF_FLUSH failed error %#x", psp_ret);
return rc;
+ }
dev_dbg(sev->dev, "SEV firmware initialized\n");
@@ -498,12 +500,12 @@ static int __sev_platform_init_locked(int *error)
return 0;
}
-int sev_platform_init(int *error)
+int sev_platform_init(void)
{
int rc;
mutex_lock(&sev_cmd_mutex);
- rc = __sev_platform_init_locked(error);
+ rc = __sev_platform_init_locked();
mutex_unlock(&sev_cmd_mutex);
return rc;
@@ -610,7 +612,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
return -EPERM;
if (sev->state == SEV_STATE_UNINIT) {
- rc = __sev_platform_init_locked(&argp->error);
+ rc = __sev_platform_init_locked();

... like here. So I don't think that should be removed.

Thanks,
Tom

if (rc)
return rc;
}
@@ -653,7 +655,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
cmd:
if (sev->state == SEV_STATE_UNINIT) {
- ret = __sev_platform_init_locked(&argp->error);
+ ret = __sev_platform_init_locked();
if (ret)
goto e_free_blob;
}
@@ -849,7 +851,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
/* If platform is not in INIT state then transition it to INIT */
if (sev->state != SEV_STATE_INIT) {
- ret = __sev_platform_init_locked(&argp->error);
+ ret = __sev_platform_init_locked();
if (ret)
goto e_free_oca;
}
@@ -973,7 +975,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
if (!writable)
return -EPERM;
- ret = __sev_platform_init_locked(&argp->error);
+ ret = __sev_platform_init_locked();
if (ret)
return ret;
}
@@ -1335,10 +1337,9 @@ void sev_pci_init(void)
return;
/* Initialize the platform */
- rc = sev_platform_init(&error);
+ rc = sev_platform_init();
if (rc)
- dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
- error, rc);
+ dev_err(sev->dev, "SEV: failed to INIT rc %d\n", rc);
return;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 1595088c428b..2f8681b753d0 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -536,8 +536,6 @@ struct sev_data_attestation_report {
/**
* sev_platform_init - perform SEV INIT command
*
- * @error: SEV command return code
- *
* Returns:
* 0 if the SEV successfully processed the command
* -%ENODEV if the SEV device is not available
@@ -545,7 +543,7 @@ struct sev_data_attestation_report {
* -%ETIMEDOUT if the SEV command timed out
* -%EIO if the SEV returned a non-zero return code
*/
-int sev_platform_init(int *error);
+int sev_platform_init(void);
/**
* sev_platform_status - perform SEV PLATFORM_STATUS command