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

[PATCH 0/4] 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-v1

and is based on cryptodev-2.6 master.


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


----------------------------------------------------------------
Ackerley Tng (3):
      crypto: ccp - Shut down SEV firmware before copying to user
      crypto: ccp - Check SNP initialization state after copying user inputs
      crypto: ccp - Make usage of sev_cmd_mutex more granular

Michael Roth (1):
      crypto: ccp - Allocate status page outside sev_cmd_mutex in SNP_PLATFORM_STATUS

 drivers/crypto/ccp/sev-dev.c | 165 ++++++++++++++++++++++++-------------------
 1 file changed, 94 insertions(+), 71 deletions(-)

[PATCH 1/4] crypto: ccp - Shut down SEV firmware before copying to user

[Michael Roth]

From: Ackerley Tng <ackerleytng@google.com>

When servicing PEK CSR and PDH certificate export ioctl commands while
the SEV platform is in the uninitialized state, the driver temporarily
transitions the firmware to the initialized state, executes the
firmware command, and shuts the firmware back down.

Currently, the firmware shutdown is performed in the cleanup path
after copying the command results back to userspace. To allow
releasing the global SEV command mutex before copying results to
userspace in a subsequent change, the temporary firmware state
transition and shutdown must remain grouped with the firmware command
execution rather than spanning across userspace memory copies.

Move the conditional firmware shutdown immediately after executing the
firmware command, before copying the output buffers to userspace.

No functional change intended.

Signed-off-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f833cb7e4da3..7de3b6bf71e1 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2022,6 +2022,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
 
+	if (shutdown_required)
+		__sev_firmware_shutdown(sev, false);
+
 	/*
 	 * 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 +2045,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;
 }
@@ -2505,6 +2505,9 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
 
+	if (shutdown_required)
+		__sev_firmware_shutdown(sev, false);
+
 	/*
 	 * 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 +2538,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);
-- 
2.43.0

Re: [PATCH 1/4] crypto: ccp - Shut down SEV firmware before copying to user

[Tom Lendacky]

On 10/14/26 08:20, Michael Roth wrote:
> From: Ackerley Tng <ackerleytng@google.com>
> 
> When servicing PEK CSR and PDH certificate export ioctl commands while
> the SEV platform is in the uninitialized state, the driver temporarily
> transitions the firmware to the initialized state, executes the
> firmware command, and shuts the firmware back down.
> 
> Currently, the firmware shutdown is performed in the cleanup path
> after copying the command results back to userspace. To allow
> releasing the global SEV command mutex before copying results to
> userspace in a subsequent change, the temporary firmware state
> transition and shutdown must remain grouped with the firmware command
> execution rather than spanning across userspace memory copies.
> 
> Move the conditional firmware shutdown immediately after executing the
> firmware command, before copying the output buffers to userspace.
> 
> No functional change intended.
> 
> 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 | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index f833cb7e4da3..7de3b6bf71e1 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2022,6 +2022,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>  
>  	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
>  
> +	if (shutdown_required)
> +		__sev_firmware_shutdown(sev, false);
> +
>  	/*
>  	 * 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 +2045,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;
>  }
> @@ -2505,6 +2505,9 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>  
>  	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
>  
> +	if (shutdown_required)
> +		__sev_firmware_shutdown(sev, false);
> +
>  	/*
>  	 * 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 +2538,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);

[PATCH 2/4] crypto: ccp - Check SNP initialization state after copying user inputs

[Michael Roth]

From: Ackerley Tng <ackerleytng@google.com>

When servicing SNP configuration and VLEK load ioctl commands, the
driver checks whether SNP is initialized before copying command inputs
from userspace.

The SNP initialization state is protected by the global SEV command
mutex. To prepare for acquiring the global SEV command mutex only
around firmware command execution rather than across userspace memory
copies, move the SNP initialization check after the userspace copies
and buffer allocations, right before issuing the firmware command.

No functional change intended.

Signed-off-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 7de3b6bf71e1..e42cf3aee50a 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2639,12 +2639,12 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable
 	if (!writable)
 		return -EPERM;
 
-	if (!sev->snp_initialized)
-		return -ENODEV;
-
 	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
 		return -EFAULT;
 
+	if (!sev->snp_initialized)
+		return -ENODEV;
+
 	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
 }
 
@@ -2661,9 +2661,6 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 	if (!writable)
 		return -EPERM;
 
-	if (!sev->snp_initialized)
-		return -ENODEV;
-
 	if (copy_from_user(&input, u64_to_user_ptr(argp->data), sizeof(input)))
 		return -EFAULT;
 
@@ -2677,7 +2674,11 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 
 	input.vlek_wrapped_address = __psp_pa(blob);
 
-	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
+	if (!sev->snp_initialized)
+		ret = -ENODEV;
+	else
+		ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
+
 	kfree(blob);
 
 	return ret;
-- 
2.43.0

Re: [PATCH 2/4] crypto: ccp - Check SNP initialization state after copying user inputs

[Tom Lendacky]

On 10/14/26 08:20, Michael Roth wrote:
> From: Ackerley Tng <ackerleytng@google.com>
> 
> When servicing SNP configuration and VLEK load ioctl commands, the
> driver checks whether SNP is initialized before copying command inputs
> from userspace.
> 
> The SNP initialization state is protected by the global SEV command
> mutex. To prepare for acquiring the global SEV command mutex only
> around firmware command execution rather than across userspace memory
> copies, move the SNP initialization check after the userspace copies
> and buffer allocations, right before issuing the firmware command.
> 
> No functional change intended.
> 
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Minor comment below only if you end up having to do a v2.

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

> ---
>  drivers/crypto/ccp/sev-dev.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 7de3b6bf71e1..e42cf3aee50a 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2639,12 +2639,12 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable
>  	if (!writable)
>  		return -EPERM;
>  
> -	if (!sev->snp_initialized)
> -		return -ENODEV;
> -
>  	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
>  		return -EFAULT;
>  
> +	if (!sev->snp_initialized)
> +		return -ENODEV;
> +
>  	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
>  }
>  
> @@ -2661,9 +2661,6 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
>  	if (!writable)
>  		return -EPERM;
>  
> -	if (!sev->snp_initialized)
> -		return -ENODEV;
> -
>  	if (copy_from_user(&input, u64_to_user_ptr(argp->data), sizeof(input)))
>  		return -EFAULT;
>  
> @@ -2677,7 +2674,11 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
>  
>  	input.vlek_wrapped_address = __psp_pa(blob);
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
> +	if (!sev->snp_initialized)
> +		ret = -ENODEV;
> +	else
> +		ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
> +

I would prefer this to be

	ret = sev->snp_initialized ? __sev_do_cmd_locked(...
				   : -ENODEV;

or

	if (sev->snp_initialized)
		ret = __sev_do_cmd_locked(...
	else
		ret = -ENODEV;

Either of those reads a bit clearer to me than the negative check with
an else - just my opinion though.

Thanks,
Tom

>  	kfree(blob);
>  
>  	return ret;

[PATCH 3/4] crypto: ccp - Make usage of sev_cmd_mutex more granular

[Michael Roth]

From: Ackerley Tng <ackerleytng@google.com>

The current code makes very coarse usage of the sev_cmd_mutex; for SEV
firmware ioctls, it is essentially taking 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
target of said 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 only around the
platform state transitions and firmware command execution.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ackerley Tng <ackerleytng@google.com>
[mdr: rework commit message]
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 98 ++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 44 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e42cf3aee50a..c8c229a6757c 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;
 
+	guard(mutex)(&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
@@ -1946,7 +1948,7 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 
 	memset(&data, 0, sizeof(data));
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
+	ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
 	if (ret)
 		return ret;
 
@@ -1965,6 +1967,8 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
 	if (!writable)
 		return -EPERM;
 
+	guard(mutex)(&sev_cmd_mutex);
+
 	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		rc = sev_move_to_init_state(argp, &shutdown_required);
 		if (rc)
@@ -2014,16 +2018,18 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	data.len = input.length;
 
 cmd:
-	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
-		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			goto e_free_blob;
-	}
+	scoped_guard(mutex, &sev_cmd_mutex) {
+		if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
+			ret = sev_move_to_init_state(argp, &shutdown_required);
+			if (ret)
+				goto e_free_blob;
+		}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
+		ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
 
-	if (shutdown_required)
-		__sev_firmware_shutdown(sev, false);
+		if (shutdown_required)
+			__sev_firmware_shutdown(sev, false);
+	}
 
 	/*
 	 * Firmware will returns the length of the CSR blob (either the minimum
@@ -2317,18 +2323,20 @@ 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;
 
-	/* 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)
-			goto e_free_oca;
-	}
+	scoped_guard(mutex, &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)
+				goto e_free_oca;
+		}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
+		ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
 e_free_oca:
-	if (shutdown_required)
-		__sev_firmware_shutdown(sev, false);
+		if (shutdown_required)
+			__sev_firmware_shutdown(sev, false);
+	}
 
 	kfree(oca_blob);
 e_free_pek:
@@ -2372,7 +2380,7 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 		data.len = 0;
 	}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, &data, &argp->error);
+	ret = sev_do_cmd(SEV_CMD_GET_ID, &data, &argp->error);
 
 	/*
 	 * Firmware will return the length of the ID value (either the minimum
@@ -2430,7 +2438,7 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
 	data->address = __psp_pa(id_blob);
 	data->len = user_size;
 
-	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
+	ret = sev_do_cmd(SEV_CMD_GET_ID, data, &argp->error);
 	if (!ret) {
 		if (copy_to_user((void __user *)argp->data, id_blob, data->len))
 			ret = -EFAULT;
@@ -2492,21 +2500,23 @@ 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->sev_plat_status.state != SEV_STATE_INIT) {
-		if (!writable) {
-			ret = -EPERM;
-			goto e_free_cert;
+	scoped_guard(mutex, &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;
+				goto e_free_cert;
+			}
+			ret = sev_move_to_init_state(argp, &shutdown_required);
+			if (ret)
+				goto e_free_cert;
 		}
-		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			goto e_free_cert;
-	}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
+		ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
 
-	if (shutdown_required)
-		__sev_firmware_shutdown(sev, false);
+		if (shutdown_required)
+			__sev_firmware_shutdown(sev, false);
+	}
 
 	/*
 	 * Firmware will return the length of the blobs (either the minimum
@@ -2605,7 +2615,8 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 	if (!argp->data)
 		return -EINVAL;
 
-	ret = __sev_do_snp_platform_status(&status, &argp->error);
+	scoped_guard(mutex, &sev_cmd_mutex)
+		ret = __sev_do_snp_platform_status(&status, &argp->error);
 	if (ret < 0)
 		return ret;
 
@@ -2623,7 +2634,7 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
 
 	buf.len = sizeof(buf);
 
-	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
+	ret = sev_do_cmd(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
 
 	return ret;
 }
@@ -2642,6 +2653,8 @@ 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;
 
+	guard(mutex)(&sev_cmd_mutex);
+
 	if (!sev->snp_initialized)
 		return -ENODEV;
 
@@ -2674,10 +2687,12 @@ 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 = -ENODEV;
-	else
-		ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
+	scoped_guard(mutex, &sev_cmd_mutex) {
+		if (!sev->snp_initialized)
+			ret = -ENODEV;
+		else
+			ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
+	}
 
 	kfree(blob);
 
@@ -2703,8 +2718,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:
@@ -2748,14 +2761,11 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
 		ret = sev_ioctl_do_snp_vlek_load(&input, writable);
 		break;
 	default:
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
 		ret = -EFAULT;
-out:
-	mutex_unlock(&sev_cmd_mutex);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH 3/4] crypto: ccp - Make usage of sev_cmd_mutex more granular

[Tom Lendacky]

On 10/14/26 08:20, Michael Roth wrote:
> From: Ackerley Tng <ackerleytng@google.com>
> 
> The current code makes very coarse usage of the sev_cmd_mutex; for SEV
> firmware ioctls, it is essentially taking 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
> target of said 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 only around the
> platform state transitions and firmware command execution.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> [mdr: rework commit message]
> Signed-off-by: Michael Roth <michael.roth@amd.com>

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

> ---
>  drivers/crypto/ccp/sev-dev.c | 98 ++++++++++++++++++++----------------
>  1 file changed, 54 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e42cf3aee50a..c8c229a6757c 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;
>  
> +	guard(mutex)(&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
> @@ -1946,7 +1948,7 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
>  
>  	memset(&data, 0, sizeof(data));
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
> +	ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
>  	if (ret)
>  		return ret;
>  
> @@ -1965,6 +1967,8 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
>  	if (!writable)
>  		return -EPERM;
>  
> +	guard(mutex)(&sev_cmd_mutex);
> +
>  	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
>  		rc = sev_move_to_init_state(argp, &shutdown_required);
>  		if (rc)
> @@ -2014,16 +2018,18 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>  	data.len = input.length;
>  
>  cmd:
> -	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
> -		ret = sev_move_to_init_state(argp, &shutdown_required);
> -		if (ret)
> -			goto e_free_blob;
> -	}
> +	scoped_guard(mutex, &sev_cmd_mutex) {
> +		if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
> +			ret = sev_move_to_init_state(argp, &shutdown_required);
> +			if (ret)
> +				goto e_free_blob;
> +		}
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
> +		ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
>  
> -	if (shutdown_required)
> -		__sev_firmware_shutdown(sev, false);
> +		if (shutdown_required)
> +			__sev_firmware_shutdown(sev, false);
> +	}
>  
>  	/*
>  	 * Firmware will returns the length of the CSR blob (either the minimum
> @@ -2317,18 +2323,20 @@ 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;
>  
> -	/* 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)
> -			goto e_free_oca;
> -	}
> +	scoped_guard(mutex, &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)
> +				goto e_free_oca;
> +		}
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
> +		ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
>  
>  e_free_oca:
> -	if (shutdown_required)
> -		__sev_firmware_shutdown(sev, false);
> +		if (shutdown_required)
> +			__sev_firmware_shutdown(sev, false);
> +	}
>  
>  	kfree(oca_blob);
>  e_free_pek:
> @@ -2372,7 +2380,7 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
>  		data.len = 0;
>  	}
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, &data, &argp->error);
> +	ret = sev_do_cmd(SEV_CMD_GET_ID, &data, &argp->error);
>  
>  	/*
>  	 * Firmware will return the length of the ID value (either the minimum
> @@ -2430,7 +2438,7 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
>  	data->address = __psp_pa(id_blob);
>  	data->len = user_size;
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
> +	ret = sev_do_cmd(SEV_CMD_GET_ID, data, &argp->error);
>  	if (!ret) {
>  		if (copy_to_user((void __user *)argp->data, id_blob, data->len))
>  			ret = -EFAULT;
> @@ -2492,21 +2500,23 @@ 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->sev_plat_status.state != SEV_STATE_INIT) {
> -		if (!writable) {
> -			ret = -EPERM;
> -			goto e_free_cert;
> +	scoped_guard(mutex, &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;
> +				goto e_free_cert;
> +			}
> +			ret = sev_move_to_init_state(argp, &shutdown_required);
> +			if (ret)
> +				goto e_free_cert;
>  		}
> -		ret = sev_move_to_init_state(argp, &shutdown_required);
> -		if (ret)
> -			goto e_free_cert;
> -	}
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
> +		ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
>  
> -	if (shutdown_required)
> -		__sev_firmware_shutdown(sev, false);
> +		if (shutdown_required)
> +			__sev_firmware_shutdown(sev, false);
> +	}
>  
>  	/*
>  	 * Firmware will return the length of the blobs (either the minimum
> @@ -2605,7 +2615,8 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>  	if (!argp->data)
>  		return -EINVAL;
>  
> -	ret = __sev_do_snp_platform_status(&status, &argp->error);
> +	scoped_guard(mutex, &sev_cmd_mutex)
> +		ret = __sev_do_snp_platform_status(&status, &argp->error);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -2623,7 +2634,7 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
>  
>  	buf.len = sizeof(buf);
>  
> -	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
> +	ret = sev_do_cmd(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
>  
>  	return ret;
>  }
> @@ -2642,6 +2653,8 @@ 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;
>  
> +	guard(mutex)(&sev_cmd_mutex);
> +
>  	if (!sev->snp_initialized)
>  		return -ENODEV;
>  
> @@ -2674,10 +2687,12 @@ 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 = -ENODEV;
> -	else
> -		ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
> +	scoped_guard(mutex, &sev_cmd_mutex) {
> +		if (!sev->snp_initialized)
> +			ret = -ENODEV;
> +		else
> +			ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
> +	}
>  
>  	kfree(blob);
>  
> @@ -2703,8 +2718,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:
> @@ -2748,14 +2761,11 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>  		ret = sev_ioctl_do_snp_vlek_load(&input, writable);
>  		break;
>  	default:
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>  	}
>  
>  	if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
>  		ret = -EFAULT;
> -out:
> -	mutex_unlock(&sev_cmd_mutex);
>  
>  	return ret;
>  }

[PATCH 4/4] crypto: ccp - Allocate status page outside sev_cmd_mutex in SNP_PLATFORM_STATUS

[Michael Roth]

Allocating memory while holding the global SEV command mutex
(sev_cmd_mutex) unnecessarily blocks concurrent SEV and SNP operations
across the system if page allocation stalls or triggers memory reclaim.

Rename __sev_do_snp_platform_status() to sev_do_snp_platform_status()
and push the sev_cmd_mutex and firmware call down into a new routine so
that allocation/freeing of the status page can be done outside of the
mutex.

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 | 60 +++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index c8c229a6757c..37b948a81bc9 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2554,20 +2554,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	return ret;
 }
 
-static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
-					int *error)
+static int __sev_do_snp_platform_status(void *data, int *error, bool *free)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_data_snp_addr buf;
-	struct page *status_page;
-	void *data;
 	int ret;
 
-	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
-	if (!status_page)
-		return -ENOMEM;
-
-	data = page_address(status_page);
+	*free = true;
 
 	/*
 	 * SNP_PLATFORM_STATUS can be executed in any SNP state. But if executed
@@ -2578,10 +2571,8 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 		 * Firmware expects the status page to be in Firmware state,
 		 * otherwise it will report an error INVALID_PAGE_STATE.
 		 */
-		if (rmp_mark_pages_firmware(__pa(data), 1, true)) {
-			ret = -EFAULT;
-			goto cleanup;
-		}
+		if (rmp_mark_pages_firmware(__pa(data), 1, true))
+			return -EFAULT;
 	}
 
 	buf.address = __psp_pa(data);
@@ -2591,19 +2582,45 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 		/*
 		 * The status page will be in Reclaim state on success, or left
 		 * in Firmware state on failure. Use snp_reclaim_pages() to
-		 * transition either case back to Hypervisor-owned state.
+		 * transition either case back to Hypervisor-owned state. If
+		 * for some reason that fails, it will have been added to the
+		 * leaked pages list, so indicate to the caller to not free
+		 * the page.
 		 */
-		if (snp_reclaim_pages(__pa(data), 1, true))
+		if (snp_reclaim_pages(__pa(data), 1, true)) {
+			*free = false;
 			return -EFAULT;
+		}
 	}
 
+	return ret;
+}
+
+static int sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
+				      int *error)
+{
+	struct page *status_page;
+	void *data;
+	bool free;
+	int ret;
+
+	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
+	if (!status_page)
+		return -ENOMEM;
+
+	data = page_address(status_page);
+
+	scoped_guard(mutex, &sev_cmd_mutex)
+		ret = __sev_do_snp_platform_status(data, error, &free);
+
 	if (ret)
 		goto cleanup;
 
 	memcpy(status, data, sizeof(*status));
 
 cleanup:
-	__free_pages(status_page, 0);
+	if (free)
+		__free_pages(status_page, 0);
 	return ret;
 }
 
@@ -2615,8 +2632,7 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 	if (!argp->data)
 		return -EINVAL;
 
-	scoped_guard(mutex, &sev_cmd_mutex)
-		ret = __sev_do_snp_platform_status(&status, &argp->error);
+	ret = sev_do_snp_platform_status(&status, &argp->error);
 	if (ret < 0)
 		return ret;
 
@@ -3114,9 +3130,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);
+	ret = sev_do_snp_platform_status(&status, &error);
 	if (ret < 0)
 		return ret;
 
@@ -3131,9 +3145,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);
+	ret = sev_do_snp_platform_status(&status, &error);
 	if (ret < 0)
 		return ret;
 
-- 
2.43.0

Re: [PATCH 4/4] crypto: ccp - Allocate status page outside sev_cmd_mutex in SNP_PLATFORM_STATUS

[Tom Lendacky]

On 10/14/26 08:20, Michael Roth wrote:
> Allocating memory while holding the global SEV command mutex
> (sev_cmd_mutex) unnecessarily blocks concurrent SEV and SNP operations
> across the system if page allocation stalls or triggers memory reclaim.
> 
> Rename __sev_do_snp_platform_status() to sev_do_snp_platform_status()
> and push the sev_cmd_mutex and firmware call down into a new routine so
> that allocation/freeing of the status page can be done outside of the
> mutex.

Seems a bit complex. Can you rename __sev_do_snp_platform_status() to
sev_do_snp_platform_status() and just put a scoped guard in after the
allocation up to just before the "if (ret) goto cleanup"?

Thanks,
Tom

> 
> 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 | 60 +++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index c8c229a6757c..37b948a81bc9 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2554,20 +2554,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>  	return ret;
>  }
>  
> -static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
> -					int *error)
> +static int __sev_do_snp_platform_status(void *data, int *error, bool *free)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
>  	struct sev_data_snp_addr buf;
> -	struct page *status_page;
> -	void *data;
>  	int ret;
>  
> -	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
> -	if (!status_page)
> -		return -ENOMEM;
> -
> -	data = page_address(status_page);
> +	*free = true;
>  
>  	/*
>  	 * SNP_PLATFORM_STATUS can be executed in any SNP state. But if executed
> @@ -2578,10 +2571,8 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
>  		 * Firmware expects the status page to be in Firmware state,
>  		 * otherwise it will report an error INVALID_PAGE_STATE.
>  		 */
> -		if (rmp_mark_pages_firmware(__pa(data), 1, true)) {
> -			ret = -EFAULT;
> -			goto cleanup;
> -		}
> +		if (rmp_mark_pages_firmware(__pa(data), 1, true))
> +			return -EFAULT;
>  	}
>  
>  	buf.address = __psp_pa(data);
> @@ -2591,19 +2582,45 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
>  		/*
>  		 * The status page will be in Reclaim state on success, or left
>  		 * in Firmware state on failure. Use snp_reclaim_pages() to
> -		 * transition either case back to Hypervisor-owned state.
> +		 * transition either case back to Hypervisor-owned state. If
> +		 * for some reason that fails, it will have been added to the
> +		 * leaked pages list, so indicate to the caller to not free
> +		 * the page.
>  		 */
> -		if (snp_reclaim_pages(__pa(data), 1, true))
> +		if (snp_reclaim_pages(__pa(data), 1, true)) {
> +			*free = false;
>  			return -EFAULT;
> +		}
>  	}
>  
> +	return ret;
> +}
> +
> +static int sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
> +				      int *error)
> +{
> +	struct page *status_page;
> +	void *data;
> +	bool free;
> +	int ret;
> +
> +	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
> +	if (!status_page)
> +		return -ENOMEM;
> +
> +	data = page_address(status_page);
> +
> +	scoped_guard(mutex, &sev_cmd_mutex)
> +		ret = __sev_do_snp_platform_status(data, error, &free);
> +
>  	if (ret)
>  		goto cleanup;
>  
>  	memcpy(status, data, sizeof(*status));
>  
>  cleanup:
> -	__free_pages(status_page, 0);
> +	if (free)
> +		__free_pages(status_page, 0);
>  	return ret;
>  }
>  
> @@ -2615,8 +2632,7 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>  	if (!argp->data)
>  		return -EINVAL;
>  
> -	scoped_guard(mutex, &sev_cmd_mutex)
> -		ret = __sev_do_snp_platform_status(&status, &argp->error);
> +	ret = sev_do_snp_platform_status(&status, &argp->error);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -3114,9 +3130,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);
> +	ret = sev_do_snp_platform_status(&status, &error);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -3131,9 +3145,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);
> +	ret = sev_do_snp_platform_status(&status, &error);
>  	if (ret < 0)
>  		return ret;
>