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

[PATCH v2 0/9] 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-v2

and is based on cryptodev-2.6 master (a29888556578).

v1: https://lore.kernel.org/linux-crypto/20261014132145.1726322-1-michael.roth@amd.com/
v2:
 - rework to address cleanup.h guidelines about not mixing goto
   with guard()'s (Sashiko)
 - invert boolean logic for better readability (Tom)
 - simplify by avoiding uncessary helper for sev_do_snp_platform_status() (Tom)


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.

Patches #7 and #9 are where the mutexes are actually made more granular,
the other patches are just prep for them.

Thank you to Sean for suggesting this approach and Ackerley for wrangling
the changes into a 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 (6):
      crypto: ccp - Use __free() handlers for sev_ioctl_do_pek_csr() allocations
      crypto: ccp - Use __free() handlers for sev_ioctl_do_pek_import() allocations
      crypto: ccp - Use __free() handlers for sev_ioctl_do_pdh_export() allocations
      crypto: ccp: - Rework sev_ioctl_do_pdh_export() to avoid goto usage
      crypto: ccp - Use __free() handlers for __sev_do_snp_platform_status() allocations
      crypto: ccp - Allocate status page outside sev_cmd_mutex in SNP_PLATFORM_STATUS

 drivers/crypto/ccp/sev-dev.c | 277 ++++++++++++++++++++-----------------------
 1 file changed, 130 insertions(+), 147 deletions(-)

[PATCH v2 1/9] 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>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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

[PATCH v2 2/9] 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>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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..ed60f3db391c 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 = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
+	else
+		ret = -ENODEV;
+
 	kfree(blob);
 
 	return ret;
-- 
2.43.0

[PATCH v2 3/9] crypto: ccp - Use __free() handlers for sev_ioctl_do_pek_csr() allocations

[Michael Roth]

Make use of __free() handlers so that the related goto statements can be
dropped. This is in preparation for switching over to using mutex
guard()'s.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ed60f3db391c..f6d06242184e 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1986,7 +1986,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	bool shutdown_required = false;
 	struct sev_data_pek_csr data;
 	void __user *input_address;
-	void *blob = NULL;
+	void *blob __free(kfree) = NULL;
 	int ret;
 
 	if (!writable)
@@ -2017,7 +2017,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	if (sev->sev_plat_status.state == SEV_STATE_UNINIT) {
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
-			goto e_free_blob;
+			return ret;
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
@@ -2031,21 +2031,17 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	 */
 	input.length = data.len;
 
-	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
-		ret = -EFAULT;
-		goto e_free_blob;
-	}
+	if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
+		return -EFAULT;
 
 	if (ret || WARN_ON_ONCE(argp->error))
-		goto e_free_blob;
+		return ret;
 
 	if (blob) {
 		if (copy_to_user(input_address, blob, input.length))
 			ret = -EFAULT;
 	}
 
-e_free_blob:
-	kfree(blob);
 	return ret;
 }
 
-- 
2.43.0

[PATCH v2 4/9] crypto: ccp - Use __free() handlers for sev_ioctl_do_pek_import() allocations

[Michael Roth]

Make use of __free() handlers so that the related goto statements can be
dropped. This is in preparation for switching over to using mutex
guard()'s.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f6d06242184e..6b483bdf0f0f 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2284,8 +2284,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;
+	void *pek_blob __free(kfree) = NULL;
+	void *oca_blob __free(kfree) = NULL;
 	bool shutdown_required = false;
-	void *pek_blob, *oca_blob;
 	int ret;
 
 	if (!writable)
@@ -2305,30 +2306,24 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 
 	/* copy PEK certificate blobs from userspace */
 	oca_blob = psp_copy_user_blob(input.oca_cert_address, input.oca_cert_len);
-	if (IS_ERR(oca_blob)) {
-		ret = PTR_ERR(oca_blob);
-		goto e_free_pek;
-	}
+	if (IS_ERR(oca_blob))
+		return PTR_ERR(oca_blob);
 
 	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) {
+	if (sev->sev_plat_status.state != SEV_STATE_INIT)
 		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			goto e_free_oca;
-	}
+	else
+		ret = 0;
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
+	if (!ret)
+		ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
-e_free_oca:
 	if (shutdown_required)
 		__sev_firmware_shutdown(sev, false);
 
-	kfree(oca_blob);
-e_free_pek:
-	kfree(pek_blob);
 	return ret;
 }
 
-- 
2.43.0

[PATCH v2 5/9] crypto: ccp - Use __free() handlers for sev_ioctl_do_pdh_export() allocations

[Michael Roth]

Make use of __free() handlers so that the related goto statements can be
dropped. This is in preparation for switching over to using mutex
guard()'s.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6b483bdf0f0f..bea3c30f00b0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2434,9 +2434,9 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
 
 static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 {
+	void *pdh_blob __free(kfree) = NULL, *cert_blob __free(kfree) = NULL;
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pdh_cert_export input;
-	void *pdh_blob = NULL, *cert_blob = NULL;
 	struct sev_data_pdh_cert_export data;
 	void __user *input_cert_chain_address;
 	void __user *input_pdh_cert_address;
@@ -2474,10 +2474,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	data.pdh_cert_len = input.pdh_cert_len;
 
 	cert_blob = kzalloc(input.cert_chain_len, GFP_KERNEL);
-	if (!cert_blob) {
-		ret = -ENOMEM;
-		goto e_free_pdh;
-	}
+	if (!cert_blob)
+		return -ENOMEM;
 
 	data.cert_chain_address = __psp_pa(cert_blob);
 	data.cert_chain_len = input.cert_chain_len;
@@ -2486,12 +2484,11 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	/* 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;
+			return -EPERM;
 		}
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
-			goto e_free_cert;
+			return ret;
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
@@ -2506,32 +2503,24 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	input.cert_chain_len = data.cert_chain_len;
 	input.pdh_cert_len = data.pdh_cert_len;
 
-	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
-		ret = -EFAULT;
-		goto e_free_cert;
-	}
+	if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
+		return -EFAULT;
 
 	if (ret || WARN_ON_ONCE(argp->error))
-		goto e_free_cert;
+		return ret;
 
 	if (pdh_blob) {
 		if (copy_to_user(input_pdh_cert_address,
-				 pdh_blob, input.pdh_cert_len)) {
-			ret = -EFAULT;
-			goto e_free_cert;
-		}
+				 pdh_blob, input.pdh_cert_len))
+			return -EFAULT;
 	}
 
 	if (cert_blob) {
 		if (copy_to_user(input_cert_chain_address,
 				 cert_blob, input.cert_chain_len))
-			ret = -EFAULT;
+			return -EFAULT;
 	}
 
-e_free_cert:
-	kfree(cert_blob);
-e_free_pdh:
-	kfree(pdh_blob);
 	return ret;
 }
 
-- 
2.43.0

[PATCH v2 6/9] crypto: ccp: - Rework sev_ioctl_do_pdh_export() to avoid goto usage

[Michael Roth]

In preparation for switching the function to use mutex guard()'s, drop
the goto by relying on an if block instead.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 49 +++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index bea3c30f00b0..4375c0480e81 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2451,41 +2451,38 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	input_pdh_cert_address = (void __user *)input.pdh_cert_address;
 	input_cert_chain_address = (void __user *)input.cert_chain_address;
 
-	/* Userspace wants to query the certificate length. */
-	if (!input.pdh_cert_address ||
-	    !input.pdh_cert_len ||
-	    !input.cert_chain_address ||
-	    !input.cert_chain_len)
-		goto cmd;
-
-	/* Allocate a physically contiguous buffer to store the PDH blob. */
-	if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)
-		return -EFAULT;
+	/* Otherwise, userspace just wants to query the certificate length. */
+	if (input.pdh_cert_address &&
+	    input.pdh_cert_len &&
+	    input.cert_chain_address &&
+	    input.cert_chain_len) {
+		/* Allocate a physically contiguous buffer to store the PDH blob. */
+		if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)
+			return -EFAULT;
 
-	/* Allocate a physically contiguous buffer to store the cert chain blob. */
-	if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)
-		return -EFAULT;
+		/* Allocate a physically contiguous buffer to store the cert chain blob. */
+		if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)
+			return -EFAULT;
 
-	pdh_blob = kzalloc(input.pdh_cert_len, GFP_KERNEL);
-	if (!pdh_blob)
-		return -ENOMEM;
+		pdh_blob = kzalloc(input.pdh_cert_len, GFP_KERNEL);
+		if (!pdh_blob)
+			return -ENOMEM;
 
-	data.pdh_cert_address = __psp_pa(pdh_blob);
-	data.pdh_cert_len = input.pdh_cert_len;
+		data.pdh_cert_address = __psp_pa(pdh_blob);
+		data.pdh_cert_len = input.pdh_cert_len;
 
-	cert_blob = kzalloc(input.cert_chain_len, GFP_KERNEL);
-	if (!cert_blob)
-		return -ENOMEM;
+		cert_blob = kzalloc(input.cert_chain_len, GFP_KERNEL);
+		if (!cert_blob)
+			return -ENOMEM;
 
-	data.cert_chain_address = __psp_pa(cert_blob);
-	data.cert_chain_len = input.cert_chain_len;
+		data.cert_chain_address = __psp_pa(cert_blob);
+		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) {
+		if (!writable)
 			return -EPERM;
-		}
 		ret = sev_move_to_init_state(argp, &shutdown_required);
 		if (ret)
 			return ret;
-- 
2.43.0

[PATCH v2 7/9] 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 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>
Signed-off-by: Ackerley Tng <ackerleytng@google.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
[mdr: rework commit message]
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 98 ++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 4375c0480e81..aae0b15a10d9 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)
-			return ret;
-	}
+	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)
+				return ret;
+		}
 
-	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
@@ -2312,17 +2318,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);
-	else
-		ret = 0;
+	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);
+		else
+			ret = 0;
 
-	if (!ret)
-		ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
+		if (!ret)
+			ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data,
+						  &argp->error);
 
-	if (shutdown_required)
-		__sev_firmware_shutdown(sev, false);
+		if (shutdown_required)
+			__sev_firmware_shutdown(sev, false);
+	}
 
 	return ret;
 }
@@ -2363,7 +2372,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
@@ -2421,7 +2430,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;
@@ -2479,19 +2488,21 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 		data.cert_chain_len = input.cert_chain_len;
 	}
 
-	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->sev_plat_status.state != SEV_STATE_INIT) {
-		if (!writable)
-			return -EPERM;
-		ret = sev_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			return ret;
-	}
+	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)
+				return -EPERM;
+			ret = sev_move_to_init_state(argp, &shutdown_required);
+			if (ret)
+				return ret;
+		}
 
-	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
@@ -2582,7 +2593,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;
 
@@ -2600,7 +2612,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;
 }
@@ -2619,6 +2631,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;
 
@@ -2651,10 +2665,13 @@ 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_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
-	else
-		ret = -ENODEV;
+	scoped_guard(mutex, &sev_cmd_mutex) {
+		if (sev->snp_initialized)
+			ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input,
+						  &argp->error);
+		else
+			ret = -ENODEV;
+	}
 
 	kfree(blob);
 
@@ -2680,8 +2697,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:
@@ -2725,14 +2740,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

[PATCH v2 8/9] crypto: ccp - Use __free() handlers for __sev_do_snp_platform_status() allocations

[Michael Roth]

Make use of __free() handlers so that the related goto statements can be
dropped. This is in preparation for switching over to using mutex
guard()'s.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index aae0b15a10d9..1e017783dd03 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2536,17 +2536,14 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 					int *error)
 {
 	struct sev_device *sev = psp_master->sev_data;
+	void *data __free(free_page) = NULL;
 	struct sev_data_snp_addr buf;
-	struct page *status_page;
-	void *data;
 	int ret;
 
-	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
-	if (!status_page)
+	data = (void *)get_zeroed_page(GFP_KERNEL_ACCOUNT);
+	if (!data)
 		return -ENOMEM;
 
-	data = page_address(status_page);
-
 	/*
 	 * 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.
@@ -2556,10 +2553,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);
@@ -2571,17 +2566,15 @@ 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)) {
+			data = NULL;
 			return -EFAULT;
+		}
 	}
 
-	if (ret)
-		goto cleanup;
-
-	memcpy(status, data, sizeof(*status));
+	if (!ret)
+		memcpy(status, data, sizeof(*status));
 
-cleanup:
-	__free_pages(status_page, 0);
 	return ret;
 }
 
-- 
2.43.0

[PATCH v2 9/9] 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. Handle allocation/freeing
outside of the mutex instead.

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 | 62 ++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 1e017783dd03..aa4f0fd731c0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2532,8 +2532,8 @@ 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(struct sev_user_data_snp_status *status,
+				      int *error)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	void *data __free(free_page) = NULL;
@@ -2544,31 +2544,36 @@ static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
 	if (!data)
 		return -ENOMEM;
 
-	/*
-	 * 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.
-	 */
-	if (sev->snp_initialized) {
+	scoped_guard(mutex, &sev_cmd_mutex) {
 		/*
-		 * Firmware expects the status page to be in Firmware state,
-		 * otherwise it will report an error INVALID_PAGE_STATE.
+		 * 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.
 		 */
-		if (rmp_mark_pages_firmware(__pa(data), 1, true))
-			return -EFAULT;
-	}
+		if (sev->snp_initialized) {
+			/*
+			 * 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))
+				return -EFAULT;
+		}
 
-	buf.address = __psp_pa(data);
-	ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
+		buf.address = __psp_pa(data);
+		ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
 
-	if (sev->snp_initialized) {
-		/*
-		 * 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.
-		 */
-		if (snp_reclaim_pages(__pa(data), 1, true)) {
-			data = NULL;
-			return -EFAULT;
+		if (sev->snp_initialized) {
+			/*
+			 * 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. 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)) {
+				data = NULL;
+				return -EFAULT;
+			}
 		}
 	}
 
@@ -2586,8 +2591,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;
 
@@ -3086,9 +3090,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;
 
@@ -3103,9 +3105,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