crypto: ccp: Sanitize sev_platform_init() error messages
Subsystems (3)
PATCH v4 1/1 crypto: ccp: Sanitize sev_platform_init() error messages
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/[email protected]/
v2:
* Address David Rientjes's feedback:
https://lore.kernel.org/all/[email protected]/
* Remove @error.
* Remove "SEV_" prefix: it is obvious from context so no need to make klog line longer.
--- drivers/crypto/ccp/sev-dev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
drivers/crypto/ccp/sev-dev.c +10 -7
@@ -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 ;
Signature · 1 line
2.38.1
Tom Lendacky
maintainerQuoted 46 lines · Jarkko Sakkinen
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 betweenDF_FLUSH, INIT andINIT_EXcommands. 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/[email protected]/v2:
* Address David Rientjes's feedback:https://lore.kernel.org/all/[email protected]/* Remove @error.
* Remove "SEV_" prefix: it is obvious from context so no need to make klog line longer.Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)drivers/crypto/ccp/sev-dev.c +0
-0
drivers/crypto/ccp/sev-dev.c static int __sev_platform_init_locked(int *error) · line 482
Quoted 8 lines
drivers/crypto/ccp/sev-dev.c +3
-1@@ -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.
drivers/crypto/ccp/sev-dev.c static int __sev_platform_init_locked(int *error) · line 490
Quoted 12 lines
drivers/crypto/ccp/sev-dev.c +2
-4- -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
Quoted 18 lines
drivers/crypto/ccp/sev-dev.c +5
-2+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 ;
Quoted 51 lines · Tom Lendacky
Quoted 50 lines · Jarkko Sakkinen
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 betweenDF_FLUSH, INIT andINIT_EXcommands. 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/[email protected]/v2:
* Address David Rientjes's feedback:https://lore.kernel.org/all/[email protected]/* Remove @error.
* Remove "SEV_" prefix: it is obvious from context so no need to make klog line longer.Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)drivers/crypto/ccp/sev-dev.c +0
-1@@ -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)
Quoted 8 lines
drivers/crypto/ccp/sev-dev.c static int __sev_platform_init_locked(int *error) · line 482
Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +3
-0+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.
Quoted 15 lines
drivers/crypto/ccp/sev-dev.c static int __sev_platform_init_locked(int *error) · line 490
Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +2
-4- -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;
Quoted 14 lines
Quoted 7 lines
drivers/crypto/ccp/sev-dev.c +4
-0+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" );drivers/crypto/ccp/sev-dev.c void sev_pci_init(void) · line 1345
Quoted 7 lines
drivers/crypto/ccp/sev-dev.c +1
-2@@ -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