| Message ID | be4d4068477c374ca6aa5b173dc1ee46ca5af44d.1735931639.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Move initializing SEV/SNP functionality to KVM | expand |
On Fri, Jan 3, 2025 at 12:00 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > From: Ashish Kalra <ashish.kalra@amd.com> > The subject includes "Fix" but has no "Fixes" tag in the commit message. > Modify the behavior of implicit SEV initialization in some of the > SEV ioctls to do both SEV initialization and shutdown and adds > implicit SNP initialization and shutdown to some of the SNP ioctls > so that the change of SEV/SNP platform initialization not being > done during PSP driver probe time does not break userspace tools > such as sevtool, etc. > It would be helpful to update the description with the state machine you're trying to maintain implicitly. I think that this changes the uapi contract as well, so I think you need to update the documentation. You have SEV shutdown on error for platform maintenance ioctls here, which already have implicit init. pdh_export gets an init if not in the init state, which wasn't already implicit because there's a wrinkle WRT the writability permission. snp_platform_status, snp_config, vlek_load, snp_commit now should be callable any time, not just when KVM has initialized SNP? If there's a caveat to the platform status, the docs need to reflect it. I don't know how SNP_COMMIT makes sense as having an implicit init/shutdown unless you're using it as SET_CONFIG, but I suppose it doesn't hurt? > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 149 +++++++++++++++++++++++++++++------ > 1 file changed, 125 insertions(+), 24 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 1c1c33d3ed9a..0ec2e8191583 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1454,7 +1454,8 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp) > static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable) > { > struct sev_device *sev = psp_master->sev_data; > - int rc; > + bool shutdown_required = false; > + int rc, ret, error; > > if (!writable) > return -EPERM; > @@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr > rc = __sev_platform_init_locked(&argp->error); > if (rc) > return rc; > + shutdown_required = true; > + } > + > + rc = __sev_do_cmd_locked(cmd, NULL, &argp->error); > + > + if (shutdown_required) { > + ret = __sev_platform_shutdown_locked(&error); > + if (ret) > + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", > + error, ret); > } > > - return __sev_do_cmd_locked(cmd, NULL, &argp->error); > + return rc; > } > > static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) > { > struct sev_device *sev = psp_master->sev_data; > struct sev_user_data_pek_csr input; > + bool shutdown_required = false; > struct sev_data_pek_csr data; > void __user *input_address; > + int ret, rc, error; > void *blob = NULL; > - int ret; > > if (!writable) > return -EPERM; > @@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) > ret = __sev_platform_init_locked(&argp->error); > if (ret) > goto e_free_blob; > + shutdown_required = true; > } > > ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error); > @@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) > } > > e_free_blob: > + if (shutdown_required) { > + rc = __sev_platform_shutdown_locked(&error); > + if (rc) > + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > kfree(blob); > return ret; > } > @@ -1739,8 +1759,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable) > struct sev_device *sev = psp_master->sev_data; > struct sev_user_data_pek_cert_import input; > struct sev_data_pek_cert_import data; > + bool shutdown_required = false; > void *pek_blob, *oca_blob; > - int ret; > + int ret, rc, error; > > if (!writable) > return -EPERM; > @@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable) > ret = __sev_platform_init_locked(&argp->error); > if (ret) > goto e_free_oca; > + shutdown_required = true; > } > > ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error); > > e_free_oca: > + if (shutdown_required) { > + rc = __sev_platform_shutdown_locked(&error); > + if (rc) > + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > kfree(oca_blob); > e_free_pek: > kfree(pek_blob); > @@ -1893,17 +1922,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) > struct sev_data_pdh_cert_export data; > void __user *input_cert_chain_address; > void __user *input_pdh_cert_address; > - int ret; > - > - /* If platform is not in INIT state then transition it to INIT. */ > - if (sev->state != SEV_STATE_INIT) { > - if (!writable) > - return -EPERM; > - > - ret = __sev_platform_init_locked(&argp->error); > - if (ret) > - return ret; > - } > + bool shutdown_required = false; > + int ret, rc, error; > > if (copy_from_user(&input, (void __user *)argp->data, sizeof(input))) > return -EFAULT; > @@ -1944,6 +1964,16 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) > data.cert_chain_len = input.cert_chain_len; > > cmd: > + /* If platform is not in INIT state then transition it to INIT. */ > + if (sev->state != SEV_STATE_INIT) { > + if (!writable) > + return -EPERM; > + ret = __sev_platform_init_locked(&argp->error); > + if (ret) > + goto e_free_cert; > + shutdown_required = true; > + } > + > ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error); > > /* If we query the length, FW responded with expected data. */ > @@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) > } > > e_free_cert: > + if (shutdown_required) { > + rc = __sev_platform_shutdown_locked(&error); > + if (rc) > + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > kfree(cert_blob); > e_free_pdh: > kfree(pdh_blob); > @@ -1979,12 +2016,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) > static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp) > { > struct sev_device *sev = psp_master->sev_data; > + bool shutdown_required = false; > struct sev_data_snp_addr buf; > struct page *status_page; > + int ret, rc, error; > void *data; > - int ret; > > - if (!sev->snp_initialized || !argp->data) > + if (!argp->data) > return -EINVAL; > > status_page = alloc_page(GFP_KERNEL_ACCOUNT); > @@ -1993,6 +2031,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp) > > data = page_address(status_page); > > + if (!sev->snp_initialized) { > + ret = __sev_snp_init_locked(&argp->error); > + if (ret) > + goto cleanup; > + shutdown_required = true; > + } > + > /* > * Firmware expects status page to be in firmware-owned state, otherwise > * it will report firmware error code INVALID_PAGE_STATE (0x1A). > @@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp) > ret = -EFAULT; > > cleanup: > + if (shutdown_required) { > + rc = __sev_snp_shutdown_locked(&error, false); > + if (rc) > + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > __free_pages(status_page, 0); > return ret; > } > @@ -2029,21 +2081,38 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp) > { > struct sev_device *sev = psp_master->sev_data; > struct sev_data_snp_commit buf; > + bool shutdown_required = false; > + int ret, rc, error; > > - if (!sev->snp_initialized) > - return -EINVAL; > + if (!sev->snp_initialized) { > + ret = __sev_snp_init_locked(&argp->error); > + if (ret) > + return ret; > + shutdown_required = true; > + } > > buf.len = sizeof(buf); > > - return __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error); > + ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error); > + > + if (shutdown_required) { > + rc = __sev_snp_shutdown_locked(&error, false); > + if (rc) > + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > + return ret; > } > > static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable) > { > struct sev_device *sev = psp_master->sev_data; > struct sev_user_data_snp_config config; > + bool shutdown_required = false; > + int ret, rc, error; > > - if (!sev->snp_initialized || !argp->data) > + if (!argp->data) > return -EINVAL; > > if (!writable) > @@ -2052,17 +2121,34 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable > if (copy_from_user(&config, (void __user *)argp->data, sizeof(config))) > return -EFAULT; > > - return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error); > + if (!sev->snp_initialized) { > + ret = __sev_snp_init_locked(&argp->error); > + if (ret) > + return ret; > + shutdown_required = true; > + } > + > + ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error); > + > + if (shutdown_required) { > + rc = __sev_snp_shutdown_locked(&error, false); > + if (rc) > + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > + return ret; > } > > static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable) > { > struct sev_device *sev = psp_master->sev_data; > struct sev_user_data_snp_vlek_load input; > + bool shutdown_required = false; > + int ret, rc, error; > void *blob; > - int ret; > > - if (!sev->snp_initialized || !argp->data) > + if (!argp->data) > return -EINVAL; > > if (!writable) > @@ -2081,8 +2167,23 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable) > > input.vlek_wrapped_address = __psp_pa(blob); > > + if (!sev->snp_initialized) { > + ret = __sev_snp_init_locked(&argp->error); > + if (ret) > + goto cleanup; > + shutdown_required = true; > + } > + > ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error); > > + if (shutdown_required) { > + rc = __sev_snp_shutdown_locked(&error, false); > + if (rc) > + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", > + error, rc); > + } > + > +cleanup: > kfree(blob); > > return ret; > -- > 2.34.1 > -- -Dionna Glaze, PhD, CISSP, CCSP (she/her)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 1c1c33d3ed9a..0ec2e8191583 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1454,7 +1454,8 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp) static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable) { struct sev_device *sev = psp_master->sev_data; - int rc; + bool shutdown_required = false; + int rc, ret, error; if (!writable) return -EPERM; @@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr rc = __sev_platform_init_locked(&argp->error); if (rc) return rc; + shutdown_required = true; + } + + rc = __sev_do_cmd_locked(cmd, NULL, &argp->error); + + if (shutdown_required) { + ret = __sev_platform_shutdown_locked(&error); + if (ret) + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", + error, ret); } - return __sev_do_cmd_locked(cmd, NULL, &argp->error); + return rc; } static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) { struct sev_device *sev = psp_master->sev_data; struct sev_user_data_pek_csr input; + bool shutdown_required = false; struct sev_data_pek_csr data; void __user *input_address; + int ret, rc, error; void *blob = NULL; - int ret; if (!writable) return -EPERM; @@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) ret = __sev_platform_init_locked(&argp->error); if (ret) goto e_free_blob; + shutdown_required = true; } ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error); @@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) } e_free_blob: + if (shutdown_required) { + rc = __sev_platform_shutdown_locked(&error); + if (rc) + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + kfree(blob); return ret; } @@ -1739,8 +1759,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable) struct sev_device *sev = psp_master->sev_data; struct sev_user_data_pek_cert_import input; struct sev_data_pek_cert_import data; + bool shutdown_required = false; void *pek_blob, *oca_blob; - int ret; + int ret, rc, error; if (!writable) return -EPERM; @@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable) ret = __sev_platform_init_locked(&argp->error); if (ret) goto e_free_oca; + shutdown_required = true; } ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error); e_free_oca: + if (shutdown_required) { + rc = __sev_platform_shutdown_locked(&error); + if (rc) + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + kfree(oca_blob); e_free_pek: kfree(pek_blob); @@ -1893,17 +1922,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) struct sev_data_pdh_cert_export data; void __user *input_cert_chain_address; void __user *input_pdh_cert_address; - int ret; - - /* If platform is not in INIT state then transition it to INIT. */ - if (sev->state != SEV_STATE_INIT) { - if (!writable) - return -EPERM; - - ret = __sev_platform_init_locked(&argp->error); - if (ret) - return ret; - } + bool shutdown_required = false; + int ret, rc, error; if (copy_from_user(&input, (void __user *)argp->data, sizeof(input))) return -EFAULT; @@ -1944,6 +1964,16 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) data.cert_chain_len = input.cert_chain_len; cmd: + /* If platform is not in INIT state then transition it to INIT. */ + if (sev->state != SEV_STATE_INIT) { + if (!writable) + return -EPERM; + ret = __sev_platform_init_locked(&argp->error); + if (ret) + goto e_free_cert; + shutdown_required = true; + } + ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error); /* If we query the length, FW responded with expected data. */ @@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) } e_free_cert: + if (shutdown_required) { + rc = __sev_platform_shutdown_locked(&error); + if (rc) + dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + kfree(cert_blob); e_free_pdh: kfree(pdh_blob); @@ -1979,12 +2016,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp) { struct sev_device *sev = psp_master->sev_data; + bool shutdown_required = false; struct sev_data_snp_addr buf; struct page *status_page; + int ret, rc, error; void *data; - int ret; - if (!sev->snp_initialized || !argp->data) + if (!argp->data) return -EINVAL; status_page = alloc_page(GFP_KERNEL_ACCOUNT); @@ -1993,6 +2031,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp) data = page_address(status_page); + if (!sev->snp_initialized) { + ret = __sev_snp_init_locked(&argp->error); + if (ret) + goto cleanup; + shutdown_required = true; + } + /* * Firmware expects status page to be in firmware-owned state, otherwise * it will report firmware error code INVALID_PAGE_STATE (0x1A). @@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp) ret = -EFAULT; cleanup: + if (shutdown_required) { + rc = __sev_snp_shutdown_locked(&error, false); + if (rc) + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + __free_pages(status_page, 0); return ret; } @@ -2029,21 +2081,38 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp) { struct sev_device *sev = psp_master->sev_data; struct sev_data_snp_commit buf; + bool shutdown_required = false; + int ret, rc, error; - if (!sev->snp_initialized) - return -EINVAL; + if (!sev->snp_initialized) { + ret = __sev_snp_init_locked(&argp->error); + if (ret) + return ret; + shutdown_required = true; + } buf.len = sizeof(buf); - return __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error); + ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error); + + if (shutdown_required) { + rc = __sev_snp_shutdown_locked(&error, false); + if (rc) + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + + return ret; } static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable) { struct sev_device *sev = psp_master->sev_data; struct sev_user_data_snp_config config; + bool shutdown_required = false; + int ret, rc, error; - if (!sev->snp_initialized || !argp->data) + if (!argp->data) return -EINVAL; if (!writable) @@ -2052,17 +2121,34 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable if (copy_from_user(&config, (void __user *)argp->data, sizeof(config))) return -EFAULT; - return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error); + if (!sev->snp_initialized) { + ret = __sev_snp_init_locked(&argp->error); + if (ret) + return ret; + shutdown_required = true; + } + + ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error); + + if (shutdown_required) { + rc = __sev_snp_shutdown_locked(&error, false); + if (rc) + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + + return ret; } static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable) { struct sev_device *sev = psp_master->sev_data; struct sev_user_data_snp_vlek_load input; + bool shutdown_required = false; + int ret, rc, error; void *blob; - int ret; - if (!sev->snp_initialized || !argp->data) + if (!argp->data) return -EINVAL; if (!writable) @@ -2081,8 +2167,23 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable) input.vlek_wrapped_address = __psp_pa(blob); + if (!sev->snp_initialized) { + ret = __sev_snp_init_locked(&argp->error); + if (ret) + goto cleanup; + shutdown_required = true; + } + ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error); + if (shutdown_required) { + rc = __sev_snp_shutdown_locked(&error, false); + if (rc) + dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", + error, rc); + } + +cleanup: kfree(blob); return ret;