Re: [PATCH v1 1/4] crypto/ccp: Do not initialize SNP for SEV ioctls

[Tom Lendacky]

On 4/27/26 11:15, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

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@xxxxxxx>

> ---
>  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;
>