public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/4] SEV re-initialization fixes
@ 2026-04-27 16:15 Tycho Andersen
  2026-04-27 16:15 ` [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls Tycho Andersen
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Tycho Andersen @ 2026-04-27 16:15 UTC (permalink / raw)
  To: Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov, Tycho Andersen (AMD)

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Here is a series targeted at fixing the bug where HSAVE_PA is cleared
after other VMs are running that Sashiko pointed out.

The first three patches are relatively straightforward, I think :). The
last one is an ABI break from how things used to work.

Thanks,

Tycho

Tycho Andersen (AMD) (4):
  crypto/ccp: Do not initialize SNP for SEV ioctls
  crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT)
  crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD)
  crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG)

 drivers/crypto/ccp/sev-dev.c | 70 ++++++------------------------------
 1 file changed, 11 insertions(+), 59 deletions(-)


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.53.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls
  2026-04-27 16:15 [PATCH v1 0/4] SEV re-initialization fixes Tycho Andersen
@ 2026-04-27 16:15 ` Tycho Andersen
  2026-04-28 21:56   ` Tom Lendacky
  2026-04-27 16:15 ` [PATCH v1 2/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT) Tycho Andersen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Tycho Andersen @ 2026-04-27 16:15 UTC (permalink / raw)
  To: Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov, Tycho Andersen (AMD)

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Sashiko notes:

> if SEV initialization fails and KVM is actively running normal VMs, could a
> userspace process trigger this code path via /dev/sev ioctls (e.g.,
> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
> execution for an active VM trigger a general protection fault and crash the
> host?

sev_move_to_init_state() is called for ioctls requiring only SEV firmware:
SEV_PEK_GEN, SEV_PDH_GEN, SEV_PEK_CSR, SEV_PEK_CERT_IMPORT, and
SEV_PDH_CERT_EXPORT. After the firmware command, it does SEV_SHUTDOWN on
the SEV firmware. Since these commands do not require SNP to be
initialized, skip it by calling __sev_platform_init_locked() which only
initializes the SEV firmware. This way SNP is not Initialized at all, and
HSAVE_PA is not cleared.

The previous code saved any SEV initialization firmware error to
init_args.error and then threw it away and hardcoded the return value of
INVALID_PLATFORM_STATE regardless of the real firmware error. This patch
changes it to surface the underlying error, which is hopefully both more
useful and doesn't cause any problems.

Note that it is still safe to call __sev_firmware_shutdown() directly: it
calls __sev_snp_shutdown_locked(), which skips SNP shutdown if SNP was not
initialized.

Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
Reported-by: Sashiko
Assisted-by: Gemini:gemini-3.1-pro-preview
Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 drivers/crypto/ccp/sev-dev.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index d1e9e0ac63b6..6891b90bbb88 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1716,14 +1716,11 @@ static int sev_get_platform_state(int *state, int *error)
 
 static int sev_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_required)
 {
-	struct sev_platform_init_args init_args = {0};
 	int rc;
 
-	rc = _sev_platform_init_locked(&init_args);
-	if (rc) {
-		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+	rc = __sev_platform_init_locked(&argp->error);
+	if (rc)
 		return rc;
-	}
 
 	*shutdown_required = true;
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v1 2/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT)
  2026-04-27 16:15 [PATCH v1 0/4] SEV re-initialization fixes Tycho Andersen
  2026-04-27 16:15 ` [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls Tycho Andersen
@ 2026-04-27 16:15 ` Tycho Andersen
  2026-04-28 21:58   ` Tom Lendacky
  2026-04-27 16:15 ` [PATCH v1 3/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD) Tycho Andersen
  2026-04-27 16:15 ` [PATCH v1 4/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG) Tycho Andersen
  3 siblings, 1 reply; 9+ messages in thread
From: Tycho Andersen @ 2026-04-27 16:15 UTC (permalink / raw)
  To: Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov, Tycho Andersen (AMD)

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Sashiko notes:

> if SEV initialization fails and KVM is actively running normal VMs, could a
> userspace process trigger this code path via /dev/sev ioctls (e.g.,
> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
> execution for an active VM trigger a general protection fault and crash the
> host?

The SNP_COMMIT command does not require the firmware to be in any
particular state. Skip initializing it if it was previously uninitialized.

The SEV-SNP firmware specification doc 56860 does not mention SNP_COMMIT in
Table 5 as a command that is allowed in the UNINIT state, but it is in fact
allowed and a future documentation update will reflect that.

Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
Reported-by: Sashiko
Assisted-by: Gemini:gemini-3.1-pro-preview
Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 drivers/crypto/ccp/sev-dev.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6891b90bbb88..572f06368d4b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2437,24 +2437,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 
 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, error;
-
-	if (!sev->snp_initialized) {
-		ret = snp_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			return ret;
-	}
+	int ret;
 
 	buf.len = sizeof(buf);
 
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
 
-	if (shutdown_required)
-		__sev_snp_shutdown_locked(&error, false);
-
 	return ret;
 }
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v1 3/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD)
  2026-04-27 16:15 [PATCH v1 0/4] SEV re-initialization fixes Tycho Andersen
  2026-04-27 16:15 ` [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls Tycho Andersen
  2026-04-27 16:15 ` [PATCH v1 2/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT) Tycho Andersen
@ 2026-04-27 16:15 ` Tycho Andersen
  2026-04-28 22:02   ` Tom Lendacky
  2026-04-27 16:15 ` [PATCH v1 4/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG) Tycho Andersen
  3 siblings, 1 reply; 9+ messages in thread
From: Tycho Andersen @ 2026-04-27 16:15 UTC (permalink / raw)
  To: Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov, Tycho Andersen (AMD)

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Sashiko notes:

> if SEV initialization fails and KVM is actively running normal VMs, could a
> userspace process trigger this code path via /dev/sev ioctls (e.g.,
> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
> execution for an active VM trigger a general protection fault and crash the
> host?

The SEV firmware docs for SNP_VLEK_LOAD note:

> On SNP_SHUTDOWN, the VLEK is deleted.

That is, the initialization/shutdown wrapper here is pointless, because the
firmware immediately throws away the key anyway. Instead, refuse to do
anything if SNP has not been previously initialized.

Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
Reported-by: Sashiko
Assisted-by: Gemini:gemini-3.1-pro-preview
Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 drivers/crypto/ccp/sev-dev.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 572f06368d4b..e8c3ac6d989a 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2481,9 +2481,8 @@ 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, error;
 	void *blob;
+	int ret;
 
 	if (!argp->data)
 		return -EINVAL;
@@ -2497,6 +2496,9 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 	if (input.len != sizeof(input) || input.vlek_wrapped_version != 0)
 		return -EINVAL;
 
+	if (!sev->snp_initialized)
+		return -EINVAL;
+
 	blob = psp_copy_user_blob(input.vlek_wrapped_address,
 				  sizeof(struct sev_user_data_snp_wrapped_vlek_hashstick));
 	if (IS_ERR(blob))
@@ -2504,18 +2506,7 @@ 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 = snp_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			goto cleanup;
-	}
-
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
-
-	if (shutdown_required)
-		__sev_snp_shutdown_locked(&error, false);
-
-cleanup:
 	kfree(blob);
 
 	return ret;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v1 4/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG)
  2026-04-27 16:15 [PATCH v1 0/4] SEV re-initialization fixes Tycho Andersen
                   ` (2 preceding siblings ...)
  2026-04-27 16:15 ` [PATCH v1 3/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD) Tycho Andersen
@ 2026-04-27 16:15 ` Tycho Andersen
  2026-04-28 22:07   ` Tom Lendacky
  3 siblings, 1 reply; 9+ messages in thread
From: Tycho Andersen @ 2026-04-27 16:15 UTC (permalink / raw)
  To: Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov, Tycho Andersen (AMD)

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Sashiko notes:

> if SEV initialization fails and KVM is actively running normal VMs, could a
> userspace process trigger this code path via /dev/sev ioctls (e.g.,
> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
> execution for an active VM trigger a general protection fault and crash the
> host?

Refuse to re-try initialization if SNP is not already initialized for
SNP_CONFIG.

This is technically an ABI break: before if SNP initialization failed it
could be transparently retriggered by this ioctl, and if no VMs were
running, everything worked fine. Hopefully this is enough of a corner case
that nobody will notice, but someone does, there are a few options:

* do something like symbol_get() for kvm and refuse to initialize if KVM is
  loaded
* check each cpu's HSAVE_PA for non-zero data before re-initializing
* once initialization has failed, continue to refuse to initialize until
  the ccp module is unloaded

Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
Reported-by: Sashiko
Assisted-by: Gemini:gemini-3.1-pro-preview
Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 drivers/crypto/ccp/sev-dev.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e8c3ac6d989a..5b113908a4f9 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1727,21 +1727,6 @@ static int sev_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_req
 	return 0;
 }
 
-static int snp_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_required)
-{
-	int error, rc;
-
-	rc = __sev_snp_init_locked(&error, 0);
-	if (rc) {
-		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
-		return rc;
-	}
-
-	*shutdown_required = true;
-
-	return 0;
-}
-
 static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
 {
 	int state, rc;
@@ -2451,8 +2436,6 @@ 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, error;
 
 	if (!argp->data)
 		return -EINVAL;
@@ -2460,21 +2443,13 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable
 	if (!writable)
 		return -EPERM;
 
+	if (!sev->snp_initialized)
+		return -EINVAL;
+
 	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
 		return -EFAULT;
 
-	if (!sev->snp_initialized) {
-		ret = snp_move_to_init_state(argp, &shutdown_required);
-		if (ret)
-			return ret;
-	}
-
-	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
-
-	if (shutdown_required)
-		__sev_snp_shutdown_locked(&error, false);
-
-	return ret;
+	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
 }
 
 static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls
  2026-04-27 16:15 ` [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls Tycho Andersen
@ 2026-04-28 21:56   ` Tom Lendacky
  0 siblings, 0 replies; 9+ messages in thread
From: Tom Lendacky @ 2026-04-28 21:56 UTC (permalink / raw)
  To: Tycho Andersen, Ashish Kalra, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov

On 4/27/26 11:15, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> Sashiko notes:
> 
>> if SEV initialization fails and KVM is actively running normal VMs, could a
>> userspace process trigger this code path via /dev/sev ioctls (e.g.,
>> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
>> execution for an active VM trigger a general protection fault and crash the
>> host?
> 
> sev_move_to_init_state() is called for ioctls requiring only SEV firmware:
> SEV_PEK_GEN, SEV_PDH_GEN, SEV_PEK_CSR, SEV_PEK_CERT_IMPORT, and
> SEV_PDH_CERT_EXPORT. After the firmware command, it does SEV_SHUTDOWN on
> the SEV firmware. Since these commands do not require SNP to be
> initialized, skip it by calling __sev_platform_init_locked() which only
> initializes the SEV firmware. This way SNP is not Initialized at all, and
> HSAVE_PA is not cleared.
> 
> The previous code saved any SEV initialization firmware error to
> init_args.error and then threw it away and hardcoded the return value of
> INVALID_PLATFORM_STATE regardless of the real firmware error. This patch
> changes it to surface the underlying error, which is hopefully both more
> useful and doesn't cause any problems.
> 
> Note that it is still safe to call __sev_firmware_shutdown() directly: it
> calls __sev_snp_shutdown_locked(), which skips SNP shutdown if SNP was not
> initialized.
> 
> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> Reported-by: Sashiko
> Assisted-by: Gemini:gemini-3.1-pro-preview
> Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>

I have a similar patch that I hadn't gotten out that added an argument to
_sev_platform_init_locked() to skip/prevent SNP initialization. I wonder
if adding something to sev_platform_init_args would be better? This could
then be expanded to prevent SNP initialization if the KVM sev_snp module
parameter was set to false.

But for a fix, this is probably simpler. It does skip some of the checks
that _sev_platform_init_locked() has, but I think all of the checks that
matter are performed for the paths that call sev_move_to_init_state().

Should this go to stable?

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

> ---
>  drivers/crypto/ccp/sev-dev.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index d1e9e0ac63b6..6891b90bbb88 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1716,14 +1716,11 @@ static int sev_get_platform_state(int *state, int *error)
>  
>  static int sev_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_required)
>  {
> -	struct sev_platform_init_args init_args = {0};
>  	int rc;
>  
> -	rc = _sev_platform_init_locked(&init_args);
> -	if (rc) {
> -		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
> +	rc = __sev_platform_init_locked(&argp->error);
> +	if (rc)
>  		return rc;
> -	}
>  
>  	*shutdown_required = true;
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 2/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT)
  2026-04-27 16:15 ` [PATCH v1 2/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT) Tycho Andersen
@ 2026-04-28 21:58   ` Tom Lendacky
  0 siblings, 0 replies; 9+ messages in thread
From: Tom Lendacky @ 2026-04-28 21:58 UTC (permalink / raw)
  To: Tycho Andersen, Ashish Kalra, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov

On 4/27/26 11:15, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> Sashiko notes:
> 
>> if SEV initialization fails and KVM is actively running normal VMs, could a
>> userspace process trigger this code path via /dev/sev ioctls (e.g.,
>> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
>> execution for an active VM trigger a general protection fault and crash the
>> host?
> 
> The SNP_COMMIT command does not require the firmware to be in any
> particular state. Skip initializing it if it was previously uninitialized.
> 
> The SEV-SNP firmware specification doc 56860 does not mention SNP_COMMIT in
> Table 5 as a command that is allowed in the UNINIT state, but it is in fact
> allowed and a future documentation update will reflect that.
> 
> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> Reported-by: Sashiko
> Assisted-by: Gemini:gemini-3.1-pro-preview
> Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>

Stable?

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

> ---
>  drivers/crypto/ccp/sev-dev.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6891b90bbb88..572f06368d4b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2437,24 +2437,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>  
>  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, error;
> -
> -	if (!sev->snp_initialized) {
> -		ret = snp_move_to_init_state(argp, &shutdown_required);
> -		if (ret)
> -			return ret;
> -	}
> +	int ret;
>  
>  	buf.len = sizeof(buf);
>  
>  	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
>  
> -	if (shutdown_required)
> -		__sev_snp_shutdown_locked(&error, false);
> -
>  	return ret;
>  }
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 3/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD)
  2026-04-27 16:15 ` [PATCH v1 3/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD) Tycho Andersen
@ 2026-04-28 22:02   ` Tom Lendacky
  0 siblings, 0 replies; 9+ messages in thread
From: Tom Lendacky @ 2026-04-28 22:02 UTC (permalink / raw)
  To: Tycho Andersen, Ashish Kalra, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov

On 4/27/26 11:15, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> Sashiko notes:
> 
>> if SEV initialization fails and KVM is actively running normal VMs, could a
>> userspace process trigger this code path via /dev/sev ioctls (e.g.,
>> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
>> execution for an active VM trigger a general protection fault and crash the
>> host?
> 
> The SEV firmware docs for SNP_VLEK_LOAD note:
> 
>> On SNP_SHUTDOWN, the VLEK is deleted.
> 
> That is, the initialization/shutdown wrapper here is pointless, because the
> firmware immediately throws away the key anyway. Instead, refuse to do
> anything if SNP has not been previously initialized.
> 
> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> Reported-by: Sashiko
> Assisted-by: Gemini:gemini-3.1-pro-preview
> Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>

Stable?

Minor comments below.

> ---
>  drivers/crypto/ccp/sev-dev.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 572f06368d4b..e8c3ac6d989a 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2481,9 +2481,8 @@ 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, error;
>  	void *blob;
> +	int ret;
>  
>  	if (!argp->data)
>  		return -EINVAL;
> @@ -2497,6 +2496,9 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
>  	if (input.len != sizeof(input) || input.vlek_wrapped_version != 0)
>  		return -EINVAL;
>  
> +	if (!sev->snp_initialized)
> +		return -EINVAL;
> +

Should this be moved up to avoid the copy_from_user()?

And should something other than -EINVAL be used, maybe -ENODEV, to help
distinguish the error a bit?

Thanks,
Tom

>  	blob = psp_copy_user_blob(input.vlek_wrapped_address,
>  				  sizeof(struct sev_user_data_snp_wrapped_vlek_hashstick));
>  	if (IS_ERR(blob))
> @@ -2504,18 +2506,7 @@ 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 = snp_move_to_init_state(argp, &shutdown_required);
> -		if (ret)
> -			goto cleanup;
> -	}
> -
>  	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
> -
> -	if (shutdown_required)
> -		__sev_snp_shutdown_locked(&error, false);
> -
> -cleanup:
>  	kfree(blob);
>  
>  	return ret;


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 4/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG)
  2026-04-27 16:15 ` [PATCH v1 4/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG) Tycho Andersen
@ 2026-04-28 22:07   ` Tom Lendacky
  0 siblings, 0 replies; 9+ messages in thread
From: Tom Lendacky @ 2026-04-28 22:07 UTC (permalink / raw)
  To: Tycho Andersen, Ashish Kalra, John Allen, Herbert Xu,
	David S. Miller
  Cc: linux-crypto, linux-kernel, Borislav Petkov

On 4/27/26 11:15, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> Sashiko notes:
> 
>> if SEV initialization fails and KVM is actively running normal VMs, could a
>> userspace process trigger this code path via /dev/sev ioctls (e.g.,
>> SEV_PDH_GEN) and zero out MSR_VM_HSAVE_PA globally? Would the next VMRUN
>> execution for an active VM trigger a general protection fault and crash the
>> host?
> 
> Refuse to re-try initialization if SNP is not already initialized for
> SNP_CONFIG.
> 
> This is technically an ABI break: before if SNP initialization failed it
> could be transparently retriggered by this ioctl, and if no VMs were
> running, everything worked fine. Hopefully this is enough of a corner case
> that nobody will notice, but someone does, there are a few options:

Isn't this the same with patch #3? Not sure why it is only called out for
this one.

> 
> * do something like symbol_get() for kvm and refuse to initialize if KVM is
>   loaded
> * check each cpu's HSAVE_PA for non-zero data before re-initializing
> * once initialization has failed, continue to refuse to initialize until
>   the ccp module is unloaded
> 
> Fixes: ceac7fb89e8d ("crypto: ccp - Ensure implicit SEV/SNP init and shutdown in ioctls")
> Reported-by: Sashiko
> Assisted-by: Gemini:gemini-3.1-pro-preview
> Link: https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> ---
>  drivers/crypto/ccp/sev-dev.c | 33 ++++-----------------------------
>  1 file changed, 4 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e8c3ac6d989a..5b113908a4f9 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1727,21 +1727,6 @@ static int sev_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_req
>  	return 0;
>  }
>  
> -static int snp_move_to_init_state(struct sev_issue_cmd *argp, bool *shutdown_required)
> -{
> -	int error, rc;
> -
> -	rc = __sev_snp_init_locked(&error, 0);
> -	if (rc) {
> -		argp->error = SEV_RET_INVALID_PLATFORM_STATE;
> -		return rc;
> -	}
> -
> -	*shutdown_required = true;
> -
> -	return 0;
> -}
> -
>  static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
>  {
>  	int state, rc;
> @@ -2451,8 +2436,6 @@ 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, error;
>  
>  	if (!argp->data)
>  		return -EINVAL;
> @@ -2460,21 +2443,13 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable
>  	if (!writable)
>  		return -EPERM;
>  
> +	if (!sev->snp_initialized)
> +		return -EINVAL;

Maybe -ENODEV to distinguish this situation?

Thanks,
Tom

> +
>  	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
>  		return -EFAULT;
>  
> -	if (!sev->snp_initialized) {
> -		ret = snp_move_to_init_state(argp, &shutdown_required);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
> -
> -	if (shutdown_required)
> -		__sev_snp_shutdown_locked(&error, false);
> -
> -	return ret;
> +	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
>  }
>  
>  static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-04-28 22:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-27 16:15 [PATCH v1 0/4] SEV re-initialization fixes Tycho Andersen
2026-04-27 16:15 ` [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls Tycho Andersen
2026-04-28 21:56   ` Tom Lendacky
2026-04-27 16:15 ` [PATCH v1 2/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_COMMIT) Tycho Andersen
2026-04-28 21:58   ` Tom Lendacky
2026-04-27 16:15 ` [PATCH v1 3/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_VLEK_LOAD) Tycho Andersen
2026-04-28 22:02   ` Tom Lendacky
2026-04-27 16:15 ` [PATCH v1 4/4] crypto/ccp: Do not initialize SNP for ioctl(SNP_CONFIG) Tycho Andersen
2026-04-28 22:07   ` Tom Lendacky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox