| 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)
On 1/6/2025 12:01 PM, Dionna Amalie Glaze wrote: > 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. > I will change the commit message to be more appropriate, it is not really a fix but a change to either add a SEV shutdown to some SEV ioctls and add SNP init and shutdown to some SNP ioctls. >> 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. > How does this change the uapi contract, as the SEV init and shutdown is going to happen as a sequence and the platform state is going to be consistent before and after the ioctl, the next ioctl if required will reissue SEV init. > 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. > This patch only adds SEV shutdown to already implied init code as part of some of these SEV ioctls. If you see the behavior prior to this patch, SEV has always been initialized before these ioctls as SEV initialization is done as part of PSP module load, but now with SEV initialization being moved to KVM module load instead of PSP driver load, the implied SEV INIT actually makes sense and gets used and additionally we need to maintain SEV platform state consistency before and after the ioctl which needs the SEV shutdown to be done after the firmware call. > 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? > Yes, and that is what this code is allowing, to call snp_platform_status, snp_config, vlek_load and snp_commit without KVM having initialized SNP. If you see the behavior prior to this patch, SNP has always been initialized before these ioctls as SNP initialization is done as part of PSP module load, therefore, to keep a consistent behavior, SNP init is being done here implicitly as part of these ioctls and then SNP shutdown before returning from the ioctl to maintain the consistent platform state before and after the ioctl. Additionally looking at the SNP firmware API specs, SNP_CONFIG needs SNP to be in INIT state. Thanks, Ashish >> 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)
On 4/1/25 07:00, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > 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. > > 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; same comment as in v2: goto e_free_cert, not return, otherwise leaks memory. > + 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); > + } > + same comment as in v2: It is the same template 8 (?) times, I'd declare rc and error inside the "if (shutdown_required)" scope or even drop them and error messages as __sev_snp_shutdown_locked() prints dev_err() anyway. if (shutdown_required) __sev_snp_shutdown_locked(&error, false); and that's it. Thanks, > +cleanup: > kfree(blob); > > return ret;
On 1/6/2025 9:29 PM, Alexey Kardashevskiy wrote: > On 4/1/25 07:00, Ashish Kalra wrote: >> From: Ashish Kalra <ashish.kalra@amd.com> >> >> 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. >> >> 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; > > same comment as in v2: > > goto e_free_cert, not return, otherwise leaks memory. > > > >> + 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); >> + } >> + > > same comment as in v2: > > > It is the same template 8 (?) times, I'd declare rc and error inside the "if (shutdown_required)" scope or even drop them and error messages as __sev_snp_shutdown_locked() prints dev_err() anyway. > > if (shutdown_required) > __sev_snp_shutdown_locked(&error, false); > > and that's it. Thanks, Yes, makes sense to use the dev_err() in __sev_snp_shutdown_locked() as that is the whole purpose of the first patch in this series, but will still have to declare error as a local as we can't use argp->error here. Thanks, Ashish > >> +cleanup: >> kfree(blob); >> return ret; >
On 1/6/2025 5:48 PM, Kalra, Ashish wrote: > > > On 1/6/2025 12:01 PM, Dionna Amalie Glaze wrote: >> 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. >> > > I will change the commit message to be more appropriate, it is not really > a fix but a change to either add a SEV shutdown to some SEV ioctls and > add SNP init and shutdown to some SNP ioctls. > >>> 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. >> > How does this change the uapi contract, as the SEV init and shutdown > is going to happen as a sequence and the platform state is going to > be consistent before and after the ioctl, the next ioctl if required > will reissue SEV init. > >> 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. >> Also note that it is important to do SEV Shutdown here with the SEV/SNP init stuff moving to KVM, if we do an implicit SEV INIT here as part of the SEV ioctls and do not follow it with SEV Shutdown then SEV will remain in INIT state and then a future SNP INIT in KVM module load will fail. This was different earlier as SNP was initialized first when CCP module is loaded, so SNP would already have been initialized when the above SEV ioctls are issued. Thanks, Ashish > > This patch only adds SEV shutdown to already implied init code as part > of some of these SEV ioctls. > > If you see the behavior prior to this patch, SEV has always been initialized > before these ioctls as SEV initialization is done as part of PSP module > load, but now with SEV initialization being moved to KVM module load instead > of PSP driver load, the implied SEV INIT actually makes sense and gets used > and additionally we need to maintain SEV platform state consistency > before and after the ioctl which needs the SEV shutdown to be done after > the firmware call. > >> 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? >> > > Yes, and that is what this code is allowing, to call snp_platform_status, > snp_config, vlek_load and snp_commit without KVM having initialized SNP. > > If you see the behavior prior to this patch, SNP has always been initialized > before these ioctls as SNP initialization is done as part of PSP module > load, therefore, to keep a consistent behavior, SNP init is being done here > implicitly as part of these ioctls and then SNP shutdown before returning > from the ioctl to maintain the consistent platform state before and > after the ioctl. > > Additionally looking at the SNP firmware API specs, SNP_CONFIG needs > SNP to be in INIT state. > > Thanks, > Ashish > >>> 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;