* [PATCH v4] crypto: ccp: Sanitize sev_platform_init() error messages
@ 2023-01-10 3:35 Jarkko Sakkinen
2023-01-10 14:41 ` Tom Lendacky
0 siblings, 1 reply; 3+ messages in thread
From: Jarkko Sakkinen @ 2023-01-10 3:35 UTC (permalink / raw)
To: Brijesh Singh, Tom Lendacky, John Allen, Herbert Xu, David S. Miller
Cc: Jarkko Sakkinen,
open list:AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER - SE...,
open list
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 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.
---
v4:
* Sorry, v3 was malformed. Here's a proper patch.
v3:
* Address Tom Lendacky's feedback:
https://lore.kernel.org/kvm/8bf6f179-eee7-fd86-7892-cdcd76e0762a@amd.com/
v2:
* Address David Rientjes's feedback:
https://lore.kernel.org/all/6a16bbe4-4281-fb28-78c4-4ec44c8aa679@google.com/
* Remove @error.
* Remove "SEV_" prefix: it is obvious from context so no need to make klog
line longer.
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
drivers/crypto/ccp/sev-dev.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 06fc7156c04f..bdc43e75c78b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -476,19 +476,23 @@ 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)
+ if (rc) {
+ dev_err(sev->dev, "SEV: %s failed error %#x",
+ sev_init_ex_buffer ? "CMD_INIT_EX" : "CMD_INIT", psp_ret);
*error = psp_ret;
-
- if (rc)
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);
+ *error = psp_ret;
return rc;
+ }
dev_dbg(sev->dev, "SEV firmware initialized\n");
@@ -1337,8 +1341,7 @@ void sev_pci_init(void)
/* Initialize the platform */
rc = sev_platform_init(&error);
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;
--
2.38.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v4] crypto: ccp: Sanitize sev_platform_init() error messages
2023-01-10 3:35 [PATCH v4] crypto: ccp: Sanitize sev_platform_init() error messages Jarkko Sakkinen
@ 2023-01-10 14:41 ` Tom Lendacky
2023-01-10 15:54 ` Jarkko Sakkinen
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lendacky @ 2023-01-10 14:41 UTC (permalink / raw)
To: Jarkko Sakkinen, Brijesh Singh, John Allen, Herbert Xu, David S. Miller
Cc: open list:AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER - SE..., open list
On 1/9/23 21:35, 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 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.
> ---
> v4:
> * Sorry, v3 was malformed. Here's a proper patch.
>
> v3:
> * Address Tom Lendacky's feedback:
> https://lore.kernel.org/kvm/8bf6f179-eee7-fd86-7892-cdcd76e0762a@amd.com/
>
> v2:
> * Address David Rientjes's feedback:
> https://lore.kernel.org/all/6a16bbe4-4281-fb28-78c4-4ec44c8aa679@google.com/
> * Remove @error.
> * Remove "SEV_" prefix: it is obvious from context so no need to make klog
> line longer.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
> ---
> drivers/crypto/ccp/sev-dev.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 06fc7156c04f..bdc43e75c78b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -476,19 +476,23 @@ 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)
> + if (rc) {
> + dev_err(sev->dev, "SEV: %s failed error %#x",
> + sev_init_ex_buffer ? "CMD_INIT_EX" : "CMD_INIT", psp_ret);
> *error = psp_ret;
If I'm not mistaken, error can be NULL, that's why the "if (error)" was
present. So that should be kept and even filled in on success. So please
leave it the way it was and just add the message to the "if (rc)" section.
> -
> - if (rc)
> 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);
Same here, add:
if (error)
*error = psp_ret;
Thanks,
Tom
> + if (rc) {
> + dev_err(sev->dev, "SEV: CMD_DF_FLUSH failed error %#x", psp_ret);
> + *error = psp_ret;
> return rc;
> + }
>
> dev_dbg(sev->dev, "SEV firmware initialized\n");
>
> @@ -1337,8 +1341,7 @@ void sev_pci_init(void)
> /* Initialize the platform */
> rc = sev_platform_init(&error);
> 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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v4] crypto: ccp: Sanitize sev_platform_init() error messages
2023-01-10 14:41 ` Tom Lendacky
@ 2023-01-10 15:54 ` Jarkko Sakkinen
0 siblings, 0 replies; 3+ messages in thread
From: Jarkko Sakkinen @ 2023-01-10 15:54 UTC (permalink / raw)
To: Tom Lendacky
Cc: Brijesh Singh, John Allen, Herbert Xu, David S. Miller,
open list:AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER - SE...,
open list
On Tue, Jan 10, 2023 at 08:41:33AM -0600, Tom Lendacky wrote:
> On 1/9/23 21:35, 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 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.
> > ---
> > v4:
> > * Sorry, v3 was malformed. Here's a proper patch.
> >
> > v3:
> > * Address Tom Lendacky's feedback:
> > https://lore.kernel.org/kvm/8bf6f179-eee7-fd86-7892-cdcd76e0762a@amd.com/
> >
> > v2:
> > * Address David Rientjes's feedback:
> > https://lore.kernel.org/all/6a16bbe4-4281-fb28-78c4-4ec44c8aa679@google.com/
> > * Remove @error.
> > * Remove "SEV_" prefix: it is obvious from context so no need to make klog
> > line longer.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
> > ---
> > drivers/crypto/ccp/sev-dev.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 06fc7156c04f..bdc43e75c78b 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -476,19 +476,23 @@ 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)
> > + if (rc) {
> > + dev_err(sev->dev, "SEV: %s failed error %#x",
> > + sev_init_ex_buffer ? "CMD_INIT_EX" : "CMD_INIT", psp_ret);
> > *error = psp_ret;
>
> If I'm not mistaken, error can be NULL, that's why the "if (error)" was
> present. So that should be kept and even filled in on success. So please
> leave it the way it was and just add the message to the "if (rc)" section.
Ah, right thanks, will do.
> > -
> > - if (rc)
> > 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);
>
> Same here, add:
>
> if (error)
> *error = psp_ret;
> > + if (rc) {
> > + dev_err(sev->dev, "SEV: CMD_DF_FLUSH failed error %#x", psp_ret);
> > + *error = psp_ret;
> > return rc;
> > + }
> > dev_dbg(sev->dev, "SEV firmware initialized\n");
> > @@ -1337,8 +1341,7 @@ void sev_pci_init(void)
> > /* Initialize the platform */
> > rc = sev_platform_init(&error);
> > 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;
BR, Jarkko
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-10 15:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-10 3:35 [PATCH v4] crypto: ccp: Sanitize sev_platform_init() error messages Jarkko Sakkinen
2023-01-10 14:41 ` Tom Lendacky
2023-01-10 15:54 ` Jarkko Sakkinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.