| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YYqhT+Enba5xa4cO@google.com>
Date: Tue, 9 Nov 2021 16:26:55 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Peter Gonda <pgonda@...gle.com>
Cc: thomas.lendacky@....com, Marc Orr <marcorr@...gle.com>,
David Rientjes <rientjes@...gle.com>,
Brijesh Singh <brijesh.singh@....com>,
Joerg Roedel <jroedel@...e.de>,
Herbert Xu <herbert@...dor.apana.org.au>,
John Allen <john.allen@....com>,
"David S. Miller" <davem@...emloft.net>,
Paolo Bonzini <pbonzini@...hat.com>,
linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
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 <pgonda@...gle.com>
> Reviewed-by: Marc Orr <marcorr@...gle.com>
> Acked-by: David Rientjes <rientjes@...gle.com>
> Acked-by: Tom Lendacky <thomas.lendacky@....com>
> Cc: Tom Lendacky <thomas.lendacky@....com>
> Cc: Brijesh Singh <brijesh.singh@....com>
> Cc: Marc Orr <marcorr@...gle.com>
> Cc: Joerg Roedel <jroedel@...e.de>
> Cc: Herbert Xu <herbert@...dor.apana.org.au>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: John Allen <john.allen@....com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: linux-crypto@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> ---
> 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, ®, 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
>
Powered by blists - more mailing lists