[PATCH v3 0/1] crypto: ccp - Make sev_cmd_mutex more granular

[PATCH v3 0/1] crypto: ccp - Make sev_cmd_mutex more granular

[Michael Roth]

This patchset is also available at:

  https://github.com/amdese/linux/commits/ccp-mutex-rework-v3

and is based on cryptodev-2.6 master (178aff4cf425).

v1: https://lore.kernel.org/linux-crypto/20261014132145.1726322-1-michael.roth@amd.com/
v2: https://lore.kernel.org/linux-crypto/20261016041109.1778673-1-michael.roth@amd.com/
v3:
 - drop all unnecessary refactorings to make patches more appropriate for 7.4
   (Herbert, Sean)
 - retest CCP both with and without kvm/queue in-place conversion support
   present, re-verify no deadlocks/regressions. Testing done primarilly with
   the following kselftest:
     https://github.com/AMDESE/linux/commit/33ddcbd756b3db5bd2bd5737e48c9084e715cd38
 - rebase on latest cryptodev master (178aff4cf425)


OVERVIEW
--------

The current code makes very coarse usage of the sev_cmd_mutex; for SEV
firmware ioctls, it is essentially taken upon entry, and ends up wrapping
a lot of potentially time-consuming prep/cleanup work that may need to be
done before/after issuing the actual firmware call, which is the main
thing the mutex is meant to protect.

Additionally, some of that prep work involves
copy_from_user()/copy_to_user() calls which can generate page faults and
unecessarily lead to potential deadlock scenarios. No known case exists
in current upstream, but with the upcoming KVM/guest_memfd support for
in-place conversion of the same PFN between shared/private state (which
is currently targeted for kernel 7.4), a special fault handler is added
that could theoretically deadlock if a malicious user tries to use
guest_memfd pages as the source/target of target of said
copy_from_user()/copy_to_user() scenarios[1].

So this series is a prereq to enabling the above-mentioned KVM/guest_memfd
in-place conversion support on SNP-enabled hosts, as well as a cleanup
for SEV ioctls in general.

Thank you to Sean for suggesting this approach and Ackerley for wrangling
the changes into a (hopefully) presentable patchset.


REFERENCES
----------

[1] https://lore.kernel.org/kvm/asz8GKNO9aQANr5E@google.com/


Thanks!

-Mike


----------------------------------------------------------------
Michael Roth (1):
      crypto: ccp - Make usage of sev_cmd_mutex more granular

 drivers/crypto/ccp/sev-dev.c | 101 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 76 insertions(+), 25 deletions(-)

[PATCH v3 1/1] crypto: ccp - Make usage of sev_cmd_mutex more granular

[Michael Roth]

The current code makes very coarse usage of the sev_cmd_mutex; for SEV
firmware ioctls, it is essentially taken upon entry, and ends up
wrapping a lot of potentially time-consuming prep work that may need to
be done before issuing the actual firmware call, which is the main thing
the mutex is meant to protect.

Additionally, some of that prep work involves
copy_from_user()/copy_to_user() calls which can generate page faults and
unecessarily lead to potential deadlock scenarios. No known case exists
in current upstream, but with the upcoming KVM/guest_memfd support for
in-place conversion of the same PFN between shared/private state, a
special fault handler is needed that could theoretically deadlock if a
malicious user tries to use guest_memfd pages as the source/target of
previously mentioned copy_from_user()/copy_to_user() scenarios.

There's no reason to hold the mutex across these problematic scenarios
however. Push sev_cmd_mutex down from the top-level sev_ioctl() into
each individual command handler, acquiring the mutex mainly around the
platform state transitions and firmware command execution.

Suggested-by: Sean Christopherson <seanjc@google.com>
Co-developed-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 101 ++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 25 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f833cb7e4da3..32610677b905 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1914,6 +1914,8 @@ static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
 	if (!writable)
 		return -EPERM;
 
+	mutex_lock(&sev_cmd_mutex);
+
 	/*
 	 * The SEV spec requires that FACTORY_RESET must be issued in
 	 * UNINIT state. Before we go further lets check if any guest is
@@ -1925,18 +1927,24 @@ static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
 	 */
 	rc = sev_get_platform_state(&state, &argp->error);
 	if (rc)
-		return rc;
+		goto out_unlock;
 
-	if (state == SEV_STATE_WORKING)
-		return -EBUSY;
+	if (state == SEV_STATE_WORKING) {
+		rc = -EBUSY;
+		goto out_unlock;
+	}
 
 	if (state == SEV_STATE_INIT) {
 		rc = __sev_platform_shutdown_locked(&argp->error);
 		if (rc)
-			return rc;
+			goto out_unlock;
 	}
 
-	return __sev_do_cmd_locked(SEV_CMD_FACTORY_RESET, NULL, &argp->error);
+	rc = __sev_do_cmd_locked(SEV_CMD_FACTORY_RESET, NULL, &argp->error);
+
+out_unlock:
+	mutex_unlock(&sev_cmd_mutex);
+	return rc;
 }
 
 static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
@@ -1946,7 +1954,9 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 
 	memset(&data, 0, sizeof(data));
 
+	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
+	mutex_unlock(&sev_cmd_mutex);
 	if (ret)
 		return ret;
 
@@ -1965,10 +1975,12 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
 	if (!writable)
 		return -EPERM;
 
+	mutex_lock(&sev_cmd_mutex);
+
 	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		rc = sev_move_to_init_state(argp, &shutdown_required);
 		if (rc)
-			return rc;
+			goto out_unlock;
 	}
 
 	rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
@@ -1976,6 +1988,9 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
 	if (shutdown_required)
 		__sev_firmware_shutdown(sev, false);
 
+out_unlock:
+	mutex_unlock(&sev_cmd_mutex);
+
 	return rc;
 }
 
@@ -2014,14 +2029,24 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	data.len = input.length;
 
 cmd:
+	mutex_lock(&sev_cmd_mutex);
+
 	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&sev_cmd_mutex);
+			/* No shutdown is needed if init fails */
 			goto e_free_blob;
+		}
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
 
+	if (shutdown_required)
+		__sev_firmware_shutdown(sev, false);
+
+	mutex_unlock(&sev_cmd_mutex);
+
 	/*
 	 * Firmware will returns the length of the CSR blob (either the minimum
 	 * required length or the actual length written), return it to the user.
@@ -2042,9 +2067,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	}
 
 e_free_blob:
-	if (shutdown_required)
-		__sev_firmware_shutdown(sev, false);
-
 	kfree(blob);
 	return ret;
 }
@@ -2317,19 +2339,26 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	data.oca_cert_address = __psp_pa(oca_blob);
 	data.oca_cert_len = input.oca_cert_len;
 
+	mutex_lock(&sev_cmd_mutex);
+
 	/* If platform is not in INIT state then transition it to INIT */
 	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&sev_cmd_mutex);
+			/* No shutdown is needed if init fails */
 			goto e_free_oca;
+		}
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
-e_free_oca:
 	if (shutdown_required)
 		__sev_firmware_shutdown(sev, false);
 
+	mutex_unlock(&sev_cmd_mutex);
+
+e_free_oca:
 	kfree(oca_blob);
 e_free_pek:
 	kfree(pek_blob);
@@ -2372,7 +2401,9 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 		data.len = 0;
 	}
 
+	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, &data, &argp->error);
+	mutex_unlock(&sev_cmd_mutex);
 
 	/*
 	 * Firmware will return the length of the ID value (either the minimum
@@ -2430,7 +2461,9 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
 	data->address = __psp_pa(id_blob);
 	data->len = user_size;
 
+	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
+	mutex_unlock(&sev_cmd_mutex);
 	if (!ret) {
 		if (copy_to_user((void __user *)argp->data, id_blob, data->len))
 			ret = -EFAULT;
@@ -2492,19 +2525,30 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	data.cert_chain_len = input.cert_chain_len;
 
 cmd:
+	mutex_lock(&sev_cmd_mutex);
+
 	/* If platform is not in INIT state then transition it to INIT. */
 	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
 		if (!writable) {
 			ret = -EPERM;
+			mutex_unlock(&sev_cmd_mutex);
 			goto e_free_cert;
 		}
 		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
+		if (ret) {
+			/* No shutdown is needed if init fails */
+			mutex_unlock(&sev_cmd_mutex);
 			goto e_free_cert;
+		}
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
 
+	if (shutdown_required)
+		__sev_firmware_shutdown(sev, false);
+
+	mutex_unlock(&sev_cmd_mutex);
+
 	/*
 	 * Firmware will return the length of the blobs (either the minimum
 	 * required length or the actual length written), return 'em to the user.
@@ -2535,9 +2579,6 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	}
 
 e_free_cert:
-	if (shutdown_required)
-		__sev_firmware_shutdown(sev, false);
-
 	kfree(cert_blob);
 e_free_pdh:
 	kfree(pdh_blob);
@@ -2559,6 +2600,8 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 
 	data = page_address(status_page);
 
+	mutex_lock(&sev_cmd_mutex);
+
 	/*
 	 * SNP_PLATFORM_STATUS can be executed in any SNP state. But if executed
 	 * when SNP has been initialized, the status page must be firmware-owned.
@@ -2570,6 +2613,7 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 		 */
 		if (rmp_mark_pages_firmware(__pa(data), 1, true)) {
 			ret = -EFAULT;
+			mutex_unlock(&sev_cmd_mutex);
 			goto cleanup;
 		}
 	}
@@ -2583,10 +2627,14 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 		 * in Firmware state on failure. Use snp_reclaim_pages() to
 		 * transition either case back to Hypervisor-owned state.
 		 */
-		if (snp_reclaim_pages(__pa(data), 1, true))
+		if (snp_reclaim_pages(__pa(data), 1, true)) {
+			mutex_unlock(&sev_cmd_mutex);
 			return -EFAULT;
+		}
 	}
 
+	mutex_unlock(&sev_cmd_mutex);
+
 	if (ret)
 		goto cleanup;
 
@@ -2623,7 +2671,9 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
 
 	buf.len = sizeof(buf);
 
+	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
+	mutex_unlock(&sev_cmd_mutex);
 
 	return ret;
 }
@@ -2632,6 +2682,7 @@ 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;
+	int ret;
 
 	if (!argp->data)
 		return -EINVAL;
@@ -2645,7 +2696,11 @@ 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);
+	mutex_lock(&sev_cmd_mutex);
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+	mutex_unlock(&sev_cmd_mutex);
+
+	return ret;
 }
 
 static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
@@ -2677,7 +2732,10 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 
 	input.vlek_wrapped_address = __psp_pa(blob);
 
+	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
+	mutex_unlock(&sev_cmd_mutex);
+
 	kfree(blob);
 
 	return ret;
@@ -2702,8 +2760,6 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
 	if (input.cmd > SEV_MAX)
 		return -EINVAL;
 
-	mutex_lock(&sev_cmd_mutex);
-
 	switch (input.cmd) {
 
 	case SEV_FACTORY_RESET:
@@ -2754,7 +2810,6 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
 	if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
 		ret = -EFAULT;
 out:
-	mutex_unlock(&sev_cmd_mutex);
 
 	return ret;
 }
@@ -3103,9 +3158,7 @@ static int get_v1_svn(struct sev_device *sev)
 	struct sev_user_data_snp_status status;
 	int ret, error = 0;
 
-	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_snp_platform_status(&status, &error);
-	mutex_unlock(&sev_cmd_mutex);
 	if (ret < 0)
 		return ret;
 
@@ -3120,9 +3173,7 @@ static int get_v2_svn(struct sev_device *sev)
 	struct sev_snp_tcb_version_turin *tcb;
 	int ret, error = 0;
 
-	mutex_lock(&sev_cmd_mutex);
 	ret = __sev_do_snp_platform_status(&status, &error);
-	mutex_unlock(&sev_cmd_mutex);
 	if (ret < 0)
 		return ret;
 
-- 
2.43.0

Re: [PATCH v3 1/1] crypto: ccp - Make usage of sev_cmd_mutex more granular

[Sean Christopherson]

On Wed, Oct 21, 2026, Michael Roth wrote:
> The current code makes very coarse usage of the sev_cmd_mutex; for SEV
> firmware ioctls, it is essentially taken upon entry, and ends up
> wrapping a lot of potentially time-consuming prep work that may need to
> be done before issuing the actual firmware call, which is the main thing
> the mutex is meant to protect.
> 
> Additionally, some of that prep work involves
> copy_from_user()/copy_to_user() calls which can generate page faults and
> unecessarily lead to potential deadlock scenarios. No known case exists
> in current upstream, but with the upcoming KVM/guest_memfd support for
> in-place conversion of the same PFN between shared/private state, a
> special fault handler is needed that could theoretically deadlock if a
> malicious user tries to use guest_memfd pages as the source/target of
> previously mentioned copy_from_user()/copy_to_user() scenarios.
> 
> There's no reason to hold the mutex across these problematic scenarios
> however. Push sev_cmd_mutex down from the top-level sev_ioctl() into
> each individual command handler, acquiring the mutex mainly around the
> platform state transitions and firmware command execution.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Co-developed-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---

Tested-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 1/1] crypto: ccp - Make usage of sev_cmd_mutex more granular

[Tom Lendacky]

On 10/21/26 14:15, Michael Roth wrote:
> The current code makes very coarse usage of the sev_cmd_mutex; for SEV
> firmware ioctls, it is essentially taken upon entry, and ends up
> wrapping a lot of potentially time-consuming prep work that may need to
> be done before issuing the actual firmware call, which is the main thing
> the mutex is meant to protect.
> 
> Additionally, some of that prep work involves
> copy_from_user()/copy_to_user() calls which can generate page faults and
> unecessarily lead to potential deadlock scenarios. No known case exists
> in current upstream, but with the upcoming KVM/guest_memfd support for
> in-place conversion of the same PFN between shared/private state, a
> special fault handler is needed that could theoretically deadlock if a
> malicious user tries to use guest_memfd pages as the source/target of
> previously mentioned copy_from_user()/copy_to_user() scenarios.
> 
> There's no reason to hold the mutex across these problematic scenarios
> however. Push sev_cmd_mutex down from the top-level sev_ioctl() into
> each individual command handler, acquiring the mutex mainly around the
> platform state transitions and firmware command execution.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Co-developed-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  drivers/crypto/ccp/sev-dev.c | 101 ++++++++++++++++++++++++++---------
>  1 file changed, 76 insertions(+), 25 deletions(-)
>