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 [off-list ref] wrote:

On Tue, Nov 02, 2021, Peter Gonda wrote:

quoted

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 <redacted>
Reviewed-by: Marc Orr <redacted>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <redacted>
Cc: Marc Orr <redacted>
Cc: Joerg Roedel <redacted>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: <redacted>
Cc: <redacted>
---
 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.