[PATCH v2 0/4] KVM: Miscellaneous SEV/SNP related fixes

[PATCH v2 0/4] KVM: Miscellaneous SEV/SNP related fixes

[Atish Patra]

This series addresses a few issues found during code audit of the
KVM SEV/SNP and CCP driver code. The fixes include a incorrect lock state
and incomplete state handling during intra-host migration for SNP VMs.

To: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
To: Borislav Petkov <bp@alien8.de>
To: Dave Hansen <dave.hansen@linux.intel.com>
To: x86@kernel.org
To: H. Peter Anvin <hpa@zytor.com>
To: Tom Lendacky <thomas.lendacky@amd.com>
To: Peter Gonda <pgonda@google.com>
To: Brijesh Singh <brijesh.singh@amd.com>
To: Youngjae Lee <youngjaelee@meta.com>
To: Ashish Kalra <ashish.kalra@amd.com>
To: Michael Roth <michael.roth@amd.com>
To: John Allen <john.allen@amd.com>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: clm@meta.com
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: stable@vger.kernel.org

Signed-off-by: Atish Patra <atishp@meta.com>
---
Changes in v2:
- Added fixes based on the reports by Sashiko. 
- Added a kselftest for validating SNP VM mirroring/migration rejection. 
- Link to v1: https://lore.kernel.org/r/20260528-sev_snp_fixes-v1-0-d67a08151779@meta.com

---
Atish Patra (4):
      KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs
      KVM: selftests: Verify SNP VMs are rejected from migration and mirroring
      crypto: ccp: Fix possible deadlock in SEV init failure path
      crypto: ccp: Fix memory leak in SEV INIT_EX path

 arch/x86/kvm/svm/sev.c                             |  4 +-
 drivers/crypto/ccp/sev-dev.c                       | 18 +++++++--
 .../testing/selftests/kvm/x86/sev_migrate_tests.c  | 47 ++++++++++++++++++++++
 3 files changed, 65 insertions(+), 4 deletions(-)
---
base-commit: e7ae89a0c97ce2b68b0983cd01eda67cf373517d
change-id: 20260525-sev_snp_fixes-0b73789c1a91

Best regards,
-- 
Atish Patra <atishp@meta.com>

[PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[Atish Patra]

From: Atish Patra <atishp@meta.com>

The intra-host migration/mirroring feature is not fully implemented for
SEV-SNP VMs. The proper migration requires additional SNP-specific
state such as guest_req_mutex, guest_req_buf, and guest_resp_buf to be
transferred or initialized on the destination.

The SNP VM mirroring requires vmsa features to be copied as well otherwise
ASID would be bound to SNP range while VM is detected as a SEV VM.

Reject SNP source VMs in migration/mirroring until proper SNP state
transfer is implemented.

Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")

Reported-by: Chris Mason <clm@meta.com>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Atish Patra <atishp@meta.com>
---
 arch/x86/kvm/svm/sev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c2126b3c3072..e6ad6af128c9 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
 		return ret;
 
 	if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
-	    sev_guest(kvm) || !sev_guest(source_kvm)) {
+	    sev_guest(kvm) || !sev_guest(source_kvm) ||
+	    sev_snp_guest(source_kvm)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
 	 * created after SEV/SEV-ES initialization, e.g. to init intercepts.
 	 */
 	if (sev_guest(kvm) || !sev_guest(source_kvm) ||
+	    sev_snp_guest(source_kvm) ||
 	    is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
 		ret = -EINVAL;
 		goto e_unlock;

-- 
2.53.0-Meta

[PATCH v2 2/4] KVM: selftests: Verify SNP VMs are rejected from migration and mirroring

[Atish Patra]

From: Atish Patra <atishp@meta.com>

Migration and mirroring of SEV-SNP VMs are not supported yet.

Add two selftests that verify KVM rejects intra-host migration and
mirroring when the source VM is an SNP VM, so the restriction stays enforced
until proper SNP state transfer is implemented.

Signed-off-by: Atish Patra <atishp@meta.com>
---
 .../testing/selftests/kvm/x86/sev_migrate_tests.c  | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86/sev_migrate_tests.c
index 6b0928e69051..acef6ab26d3d 100644
--- a/tools/testing/selftests/kvm/x86/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86/sev_migrate_tests.c
@@ -313,6 +313,49 @@ static void test_sev_mirror_parameters(void)
 	kvm_vm_free(vm_no_vcpu);
 }
 
+static void test_sev_snp_migrate_reject(void)
+{
+	struct kvm_vm *src_vm, *dst_vm;
+	int ret;
+
+	src_vm = vm_create_barebones_type(KVM_X86_SNP_VM);
+	snp_vm_init(src_vm);
+	__vm_vcpu_add(src_vm, 0);
+	vm_sev_launch(src_vm, snp_default_policy(), NULL);
+
+	dst_vm = vm_create_barebones_type(KVM_X86_SNP_VM);
+	__vm_vcpu_add(dst_vm, 0);
+
+	ret = __sev_migrate_from(dst_vm, src_vm);
+	TEST_ASSERT(ret == -1 && errno == EINVAL,
+		    "SNP VM migration should be rejected. ret: %d, errno: %d",
+		    ret, errno);
+
+	kvm_vm_free(src_vm);
+	kvm_vm_free(dst_vm);
+}
+
+static void test_sev_snp_mirror_reject(void)
+{
+	struct kvm_vm *src_vm, *dst_vm;
+	int ret;
+
+	src_vm = vm_create_barebones_type(KVM_X86_SNP_VM);
+	snp_vm_init(src_vm);
+	__vm_vcpu_add(src_vm, 0);
+	vm_sev_launch(src_vm, snp_default_policy(), NULL);
+
+	dst_vm = aux_vm_create(false);
+
+	ret = __sev_mirror_create(dst_vm, src_vm);
+	TEST_ASSERT(ret == -1 && errno == EINVAL,
+		    "SNP VM mirroring should be rejected. ret: %d, errno: %d",
+		    ret, errno);
+
+	kvm_vm_free(src_vm);
+	kvm_vm_free(dst_vm);
+}
+
 static void test_sev_move_copy(void)
 {
 	struct kvm_vm *dst_vm, *dst2_vm, *dst3_vm, *sev_vm, *mirror_vm,
@@ -384,12 +427,16 @@ int main(int argc, char *argv[])
 		test_sev_migrate_parameters();
 		if (kvm_has_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM))
 			test_sev_move_copy();
+		if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
+			test_sev_snp_migrate_reject();
 	}
 	if (kvm_has_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM)) {
 		test_sev_mirror(/* es= */ false);
 		if (have_sev_es)
 			test_sev_mirror(/* es= */ true);
 		test_sev_mirror_parameters();
+		if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
+			test_sev_snp_mirror_reject();
 	}
 	return 0;
 }

-- 
2.53.0-Meta

[PATCH v2 3/4] crypto: ccp: Fix possible deadlock in SEV init failure path

[Atish Patra]

From: Atish Patra <atishp@meta.com>

__sev_platform_init_handle_init_ex_path() called
rmp_mark_pages_firmware() with locked=false but while the parent
function of init_ex_path already acquired the sev_cmd_mutex.
In case of a rmpupdate failure for any page after the first, the cleanup
path would invoke reclaim pages which would result in a deadlock in
sev_do_cmd.

Pass locked=true to honor the lock status of the parent function.

Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")

Reported-by: Chris Mason <clm@meta.com>
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Atish Patra <atishp@meta.com>
---
 drivers/crypto/ccp/sev-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index d1e9e0ac63b6..3d4793e8e34b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1557,7 +1557,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
 		unsigned long npages;
 
 		npages = 1UL << get_order(NV_LENGTH);
-		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, false)) {
+		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
 			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
 			return -ENOMEM;
 		}

-- 
2.53.0-Meta

[PATCH v2 4/4] crypto: ccp: Fix memory leak in SEV INIT_EX path

[Atish Patra]

From: Atish Patra <atishp@meta.com>

allocated pages in _init_ext_path are never freed and sev_init_ex_buffer
is left pointing at the leaked memory in case of any failures during the
function..

Fix by adding an error path that frees the pages and clears
sev_init_ex_buffer. Make sure we only free the memory if the failure
happens before the conversion. Otherwise, we may end up trying to free
up converted pages in case of reclaim failure. rmp_mark_pages_firmware
failures should be rare enough to avoid more code complexity to track
down which pages were reclaimed/leaked vs which are not.

Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Atish Patra <atishp@meta.com>
---
 drivers/crypto/ccp/sev-dev.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 3d4793e8e34b..8566f164430b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1550,7 +1550,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
 
 	rc = sev_read_init_ex_file();
 	if (rc)
-		return rc;
+		goto err_free;
 
 	/* If SEV-SNP is initialized, transition to firmware page. */
 	if (sev->snp_initialized) {
@@ -1559,11 +1559,23 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
 		npages = 1UL << get_order(NV_LENGTH);
 		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
 			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
-			return -ENOMEM;
+			rc = -ENOMEM;
+			/*
+			 * Don't free on conversion failure: the rollback may
+			 * have left pages firmware-owned, and a high-order
+			 * block can't be partially freed.
+			 */
+			goto err_reset;
 		}
 	}
 
 	return 0;
+
+err_free:
+	__free_pages(page, get_order(NV_LENGTH));
+err_reset:
+	sev_init_ex_buffer = NULL;
+	return rc;
 }
 
 static int __sev_platform_init_locked(int *error)

-- 
2.53.0-Meta

Re: [PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `kvm->arch.vm_type` validation in `sev_vm_copy_enc_context_from` allows creating structurally inconsistent mirror VMs.
- [High] Missing `vmsa_features` copy in `sev_vm_copy_enc_context_from` breaks SEV-ES mirror VMs that use features like `DEBUG_SWAP`.
- [High] Concurrent list modification of `owner_sev_info->mirror_vms` without holding the owner VM's lock during mirror VM migration.
--

commit 5adb8a893221b0569310f07fd9fc7abca8088651
Author: Atish Patra <atishp@meta.com>

KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

This commit rejects SNP source VMs in migration/mirroring until proper SNP
state transfer is implemented.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  		return ret;
>  
>  	if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
> -	    sev_guest(kvm) || !sev_guest(source_kvm)) {
> +	    sev_guest(kvm) || !sev_guest(source_kvm) ||
> +	    sev_snp_guest(source_kvm)) {

[Severity: High]
This is a pre-existing issue, but I noticed a potential locking race when
an existing mirror VM is migrated via sev_vm_move_enc_context_from().

Locks are only acquired for the source mirror (source_kvm) and destination
VM (kvm):

arch/x86/kvm/svm/sev.c:sev_vm_move_enc_context_from() {
    ...
    ret = sev_lock_two_vms(kvm, source_kvm);
    if (ret)
        return ret;
    ...
}

Later, in sev_migrate_from(), it updates its owner's list of mirrors:

arch/x86/kvm/svm/sev.c:sev_migrate_from() {
    ...
    if (is_mirroring_enc_context(dst_kvm)) {
        struct kvm_sev_info *owner_sev_info =
            to_kvm_sev_info(dst->enc_context_owner);

        list_del(&src->mirror_entry);
        list_add_tail(&dst->mirror_entry, &owner_sev_info->mirror_vms);
    }
    ...
}

Since the required lock for enc_context_owner is omitted entirely, could
this concurrently modify the owner_sev_info->mirror_vms linked list if
userspace creates a new mirror via sev_vm_copy_enc_context_from(), which
locks the owner VM? Would this result in kernel list corruption?

>  		ret = -EINVAL;
>  		goto out_unlock;
>  	}
> @@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  	 * created after SEV/SEV-ES initialization, e.g. to init intercepts.
>  	 */
>  	if (sev_guest(kvm) || !sev_guest(source_kvm) ||
> +	    sev_snp_guest(source_kvm) ||
>  	    is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {

[Severity: High]
This isn't a bug introduced by this patch, but this block seems to be
missing the kvm->arch.vm_type != source_kvm->arch.vm_type check that
exists in sev_vm_move_enc_context_from().

Does this omission allow creating structurally inconsistent mirror VMs?
Userspace could potentially create a destination VM as KVM_X86_SNP_VM and
mirror a source VM of type KVM_X86_SEV_VM into it. Since KVM's core MMU
would treat the new VM as an SNP VM (due to arch.vm_type), but the SEV
code treats it as a legacy SEV VM (because ____sev_snp_guest evaluates
to false), could this produce an inconsistent internal state and allow
attaching guest_memfd to standard SEV VMs?

>  		ret = -EINVAL;
>  		goto e_unlock;

[Severity: High]
This is a pre-existing issue, but I noticed another detail in
sev_vm_copy_enc_context_from() just below this diff.

The initialization block for the mirror VM omits copying vmsa_features:

arch/x86/kvm/svm/sev.c:sev_vm_copy_enc_context_from() {
    ...
    mirror_sev->enc_context_owner = source_kvm;
    mirror_sev->active = true;
    mirror_sev->asid = source_sev->asid;
    mirror_sev->fd = source_sev->fd;
    mirror_sev->es_active = source_sev->es_active;
    mirror_sev->need_init = false;
    mirror_sev->handle = source_sev->handle;
    ...
}

This omits mirror_sev->vmsa_features = source_sev->vmsa_features.

Since SEV-ES VMs use vmsa_features to track capabilities like
SVM_SEV_FEAT_DEBUG_SWAP, would a mirrored SEV-ES VM incorrectly
initialize its vmsa_features to 0? When vCPUs are instantiated,
sev_es_init_vmcb() checks sev_vcpu_has_debug_swap(), which would evaluate
to false, resulting in misconfigured VMCB intercepts. Could this silently
drop important execution features for SEV-ES mirrors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-sev_snp_fixes-v2-0-611891b28a86@meta.com?part=1

Re: [PATCH v2 3/4] crypto: ccp: Fix possible deadlock in SEV init failure path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Failure in snp_prep_cmd_buf permanently leaks the command buffer active state in __sev_do_cmd_locked, deadlocking the SEV interface.
- [High] sev_init_ex_buffer is allocated without zeroing, leaking uninitialized kernel memory to the host filesystem.
- [High] Unhandled allocation failure for dev_data->output in spdm_ctrl_alloc leads to passing a null physical address to the PSP firmware.
- [High] Retrying SEV-TIO commands after buffer expansion uses stale, freed physical addresses, leading to RMP violations.
- [High] Bounce buffer memory is permanently leaked when firmware page transitions fail during legacy SEV command preparation.
--

commit 1396d09c5ba264e4a091f5fa101bb062c384525c
Author: Atish Patra <atishp@meta.com>

crypto: ccp: Fix possible deadlock in SEV init failure path

__sev_platform_init_handle_init_ex_path() called
rmp_mark_pages_firmware() with locked=false but while the parent
function of init_ex_path already acquired the sev_cmd_mutex.
In case of a rmpupdate failure for any page after the first, the cleanup
path would invoke reclaim pages which would result in a deadlock in
sev_do_cmd.

Pass locked=true to honor the lock status of the parent function.

Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
Reported-by: Chris Mason <clm@meta.com>
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Atish Patra <atishp@meta.com>

> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index d1e9e0ac63b60..3d4793e8e34b2 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1557,7 +1557,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  		unsigned long npages;
>  
>  		npages = 1UL << get_order(NV_LENGTH);
> -		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, false)) {
> +		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>  			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>  			return -ENOMEM;
>  		}

[Severity: High]
This is a pre-existing issue, but does this allocation leak uninitialized
memory?

Looking at __sev_platform_init_handle_init_ex_path() in the same file:

drivers/crypto/ccp/sev-dev.c:__sev_platform_init_handle_init_ex_path() {
    ...
    page = alloc_pages(GFP_KERNEL, get_order(NV_LENGTH));
    ...
}

If the NV file does not exist, sev_read_init_ex_file() returns 0 without
writing any data. This leaves the 32KB buffer filled with uninitialized
kernel memory before transitioning to firmware state. When
sev_write_init_ex_file() later writes this buffer to the host filesystem,
could this persist kernel heap contents to disk if the firmware did not
fully overwrite it?

[Severity: High]
This is a pre-existing issue, but can this error path leave the command
buffer permanently active?

drivers/crypto/ccp/sev-dev.c:__sev_do_cmd_locked() {
    ...
    ret = snp_prep_cmd_buf(cmd, cmd_buf, desc_list);
    if (ret) {
        dev_err(sev->dev, "SEV: failed to prepare buffer...");
        return ret;
    }
    ...
}

Since sev->cmd_buf_active or sev->cmd_buf_backup_active were set to true
before this call, returning directly here bypasses resetting those flags.
Would this cause subsequent commands to fail with -EBUSY and lock up the
interface?

[Severity: High]
This is a pre-existing issue, but does this error path leak the mapped
bounce buffers?

drivers/crypto/ccp/sev-dev.c:snp_prep_cmd_buf() {
    ...
    if (sev_cmd_buf_writable(cmd)) {
        if (rmp_mark_pages_firmware(__pa(cmd_buf), 1, true))
            return -EFAULT;
    }
    ...
}

If this fails, it returns -EFAULT without calling
snp_unmap_cmd_buf_desc_list(), which appears to leave the mapped command
descriptors unmapped.

Similarly, in snp_map_cmd_buf_desc():

drivers/crypto/ccp/sev-dev.c:snp_map_cmd_buf_desc() {
    ...
    if (rmp_mark_pages_firmware(*desc->paddr_ptr, npages, true)) {
        pr_warn("Error moving pages to firmware-owned state...");
        return -EFAULT;
    }
    ...
}

If the firmware transition fails for an individual bounce buffer, the newly
allocated page isn't freed before returning -EFAULT. Could both of these
paths result in permanent memory leaks?

[Severity: High]
This is a pre-existing issue, but does this validation correctly check the
output buffer?

drivers/crypto/ccp/sev-dev-tio.c:spdm_ctrl_alloc() {
    ...
    dev_data->output = sla_alloc(dev_data->output_len, true);

    if (IS_SLA_NULL(dev_data->req) || IS_SLA_NULL(dev_data->resp) ||
        IS_SLA_NULL(dev_data->scratch) || IS_SLA_NULL(dev_data->dev_ctx)) {
    ...
}

The check examines dev_ctx instead of the newly allocated output. If the
output allocation fails, dev_data->output will be SLA_NULL (physical frame
number 0). Will returning success here cause the PSP firmware to use
physical page 0 as an output buffer and trigger an RMP violation?

[Severity: High]
This is a pre-existing issue, but does retrying the command use stale
physical addresses?

drivers/crypto/ccp/sev-dev-tio.c:sev_tio_do_cmd() {
    ...
    rc1 = sla_expand(&dev_data->output, &dev_data->output_len);
    ...
    rc = sev_do_cmd(cmd, data, psp_ret);
    ...
}

When sla_expand() allocates a new buffer and frees the old one, dev_data
pointers are updated. However, the data pointer passed to sev_do_cmd()
comes from the caller and contains a by-value copy of the old spdm_ctrl
fields. Will retrying the command pass the old, freed physical addresses
to the PSP firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-sev_snp_fixes-v2-0-611891b28a86@meta.com?part=3

Re: [PATCH v2 4/4] crypto: ccp: Fix memory leak in SEV INIT_EX path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] When SEV-SNP is initialized, `sev_init_ex_buffer` is explicitly transitioned to a firmware-owned state via `rmp_mark_pages_firmware()`. However, the driver never reclaims this buffer back to a shared state before reading it. Immediately after the `INIT_EX` command completes, `__sev_do_cmd_locked()` unconditionally calls `sev_write_init_ex_file_if_required()`, which invokes `kernel_write()` to read from `sev_init_ex_buffer` and save it to disk. The host CPU's attempt to read firmware-owned pages inside `kernel_write()` will trigger an RMP violation (Machine Check Exception or #PF with RMP bit), causing a fatal host crash.
- [High] The `sev_init_ex_buffer` is allocated without the `__GFP_ZERO` flag. If the NV memory file does not exist (returning `-ENOENT`), or a short read occurs, `sev_read_init_ex_file()` returns 0 without fully initializing the 32KB buffer. This uninitialized kernel memory is passed to the SEV firmware via the `INIT_EX` command. Later, `sev_write_init_ex_file()` writes the entire 32KB buffer back to the host filesystem, resulting in an information leak of kernel heap data to disk.
--

commit 9089d07a571c9b175950b197e846ef7b16492229
Author: Atish Patra <atishp@meta.com>

crypto: ccp: Fix memory leak in SEV INIT_EX path

This commit fixes a memory leak in the SEV INIT_EX path by freeing
allocated pages and clearing the buffer on failure. It avoids freeing
pages if they have already been converted to firmware-owned memory.

> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1550,7 +1550,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  
>  	rc = sev_read_init_ex_file();
>  	if (rc)
> -		return rc;
> +		goto err_free;

[Severity: High]
This is a pre-existing issue, but does allocating sev_init_ex_buffer
without zeroing leak kernel memory?

Earlier in __sev_platform_init_handle_init_ex_path(), the buffer is
allocated without the zeroing flag:

page = alloc_pages(GFP_KERNEL, get_order(NV_LENGTH));

If the NV memory file does not exist, sev_read_init_ex_file() sets
ret = 0 on -ENOENT and returns without initializing the buffer. Short
reads also leave the remainder of the buffer uninitialized.

Later, sev_write_init_ex_file() writes the entire 32KB buffer back to
the host filesystem via kernel_write(). Could this result in an
information leak of uninitialized kernel heap data to disk?

>  
>  	/* If SEV-SNP is initialized, transition to firmware page. */
>  	if (sev->snp_initialized) {
> @@ -1559,11 +1559,23 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  		npages = 1UL << get_order(NV_LENGTH);
>  		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>  			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
> -			return -ENOMEM;
> +			rc = -ENOMEM;

[Severity: Critical]
This is a pre-existing issue, but does reading a firmware-owned page
from the host CPU trigger an RMP violation?

When the buffer is explicitly transitioned to a firmware-owned state
via rmp_mark_pages_firmware() here, the driver does not appear to
reclaim this buffer back to a shared state before reading it.

Immediately after the command completes, __sev_do_cmd_locked()
unconditionally calls sev_write_init_ex_file_if_required(), which
invokes kernel_write() to read from sev_init_ex_buffer and save it
to disk.

Will the host CPU's attempt to read firmware-owned pages inside
kernel_write() trigger a fatal page fault or Machine Check Exception,
crashing the host?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-sev_snp_fixes-v2-0-611891b28a86@meta.com?part=4

Re: [PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[Tom Lendacky]

On 6/1/26 18:04, Atish Patra wrote:
> From: Atish Patra <atishp@meta.com>
> 
> The intra-host migration/mirroring feature is not fully implemented for
> SEV-SNP VMs. The proper migration requires additional SNP-specific
> state such as guest_req_mutex, guest_req_buf, and guest_resp_buf to be
> transferred or initialized on the destination.
> 
> The SNP VM mirroring requires vmsa features to be copied as well otherwise
> ASID would be bound to SNP range while VM is detected as a SEV VM.
> 
> Reject SNP source VMs in migration/mirroring until proper SNP state
> transfer is implemented.
> 
> Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")

Probably not the correct Fixes: tag. It should the tag that first
introduces SNP hypervisor support.

And adding a comment above the if statements that indicate additional
support is required for SNP, so don't allow it for now, would be nice.

Otherwise, for the actual code...

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

> 
> Reported-by: Chris Mason <clm@meta.com>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Atish Patra <atishp@meta.com>
> ---
>  arch/x86/kvm/svm/sev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c2126b3c3072..e6ad6af128c9 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  		return ret;
>  
>  	if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
> -	    sev_guest(kvm) || !sev_guest(source_kvm)) {
> +	    sev_guest(kvm) || !sev_guest(source_kvm) ||
> +	    sev_snp_guest(source_kvm)) {
>  		ret = -EINVAL;
>  		goto out_unlock;
>  	}
> @@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>  	 * created after SEV/SEV-ES initialization, e.g. to init intercepts.
>  	 */
>  	if (sev_guest(kvm) || !sev_guest(source_kvm) ||
> +	    sev_snp_guest(source_kvm) ||
>  	    is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
>  		ret = -EINVAL;
>  		goto e_unlock;
> 

Re: [PATCH v2 3/4] crypto: ccp: Fix possible deadlock in SEV init failure path

[Tom Lendacky]

On 6/1/26 18:04, Atish Patra wrote:
> From: Atish Patra <atishp@meta.com>
> 
> __sev_platform_init_handle_init_ex_path() called

s/called/calls/

> rmp_mark_pages_firmware() with locked=false but while the parent

s/but//

> function of init_ex_path already acquired the sev_cmd_mutex.
> In case of a rmpupdate failure for any page after the first, the cleanup

s/In case/In the case/
s/a rmpupdate/an RMPUPDATE/

> path would invoke reclaim pages which would result in a deadlock in
> sev_do_cmd.
> 
> Pass locked=true to honor the lock status of the parent function.
> 
> Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
> 
> Reported-by: Chris Mason <clm@meta.com>
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Atish Patra <atishp@meta.com>

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

> ---
>  drivers/crypto/ccp/sev-dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index d1e9e0ac63b6..3d4793e8e34b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1557,7 +1557,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  		unsigned long npages;
>  
>  		npages = 1UL << get_order(NV_LENGTH);
> -		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, false)) {
> +		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>  			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>  			return -ENOMEM;
>  		}
> 

Re: [PATCH v2 4/4] crypto: ccp: Fix memory leak in SEV INIT_EX path

[Tom Lendacky]

On 6/1/26 18:04, Atish Patra wrote:
> From: Atish Patra <atishp@meta.com>
> 
> allocated pages in _init_ext_path are never freed and sev_init_ex_buffer
> is left pointing at the leaked memory in case of any failures during the
> function..
> 
> Fix by adding an error path that frees the pages and clears
> sev_init_ex_buffer. Make sure we only free the memory if the failure
> happens before the conversion. Otherwise, we may end up trying to free
> up converted pages in case of reclaim failure. rmp_mark_pages_firmware
> failures should be rare enough to avoid more code complexity to track
> down which pages were reclaimed/leaked vs which are not.
> 
> Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Atish Patra <atishp@meta.com>

Not sure the goto's are the best, but they do the job - just a personal
preference for me here.

The new comment below is a bit verbose, I would think it is sufficient to
just say something like "Pages can be in an inconsistent state, don't
release them back to the system" or such.

It might be nice in the future if we can identify if the reclaim was
successful and use that for determining whether the pages are safe to
freed... but the failure chance should be practically zero, so I'm not
sure it is worth it.

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

> ---
>  drivers/crypto/ccp/sev-dev.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 3d4793e8e34b..8566f164430b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1550,7 +1550,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  
>  	rc = sev_read_init_ex_file();
>  	if (rc)
> -		return rc;
> +		goto err_free;
>  
>  	/* If SEV-SNP is initialized, transition to firmware page. */
>  	if (sev->snp_initialized) {
> @@ -1559,11 +1559,23 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  		npages = 1UL << get_order(NV_LENGTH);
>  		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>  			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
> -			return -ENOMEM;
> +			rc = -ENOMEM;
> +			/*
> +			 * Don't free on conversion failure: the rollback may
> +			 * have left pages firmware-owned, and a high-order
> +			 * block can't be partially freed.
> +			 */
> +			goto err_reset;
>  		}
>  	}
>  
>  	return 0;
> +
> +err_free:
> +	__free_pages(page, get_order(NV_LENGTH));
> +err_reset:
> +	sev_init_ex_buffer = NULL;
> +	return rc;
>  }
>  
>  static int __sev_platform_init_locked(int *error)
> 

Re: [PATCH v2 4/4] crypto: ccp: Fix memory leak in SEV INIT_EX path

[Atish Patra]

On 6/2/26 7:54 AM, Tom Lendacky wrote:
> On 6/1/26 18:04, Atish Patra wrote:
>> From: Atish Patra <atishp@meta.com>
>>
>> allocated pages in _init_ext_path are never freed and sev_init_ex_buffer
>> is left pointing at the leaked memory in case of any failures during the
>> function..
>>
>> Fix by adding an error path that frees the pages and clears
>> sev_init_ex_buffer. Make sure we only free the memory if the failure
>> happens before the conversion. Otherwise, we may end up trying to free
>> up converted pages in case of reclaim failure. rmp_mark_pages_firmware
>> failures should be rare enough to avoid more code complexity to track
>> down which pages were reclaimed/leaked vs which are not.
>>
>> Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Signed-off-by: Atish Patra <atishp@meta.com>
> Not sure the goto's are the best, but they do the job - just a personal
> preference for me here.
>
> The new comment below is a bit verbose, I would think it is sufficient to
> just say something like "Pages can be in an inconsistent state, don't
> release them back to the system" or such.
Sure. I will update the comment.
> It might be nice in the future if we can identify if the reclaim was
> successful and use that for determining whether the pages are safe to
> freed... but the failure chance should be practically zero, so I'm not
> sure it is worth it.

Yes. I had started that path but was not sure if the code churn is worth it.
I can send it as a separate patch and we can take a call if you are 
think it's worth.

>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
>> ---
>>   drivers/crypto/ccp/sev-dev.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 3d4793e8e34b..8566f164430b 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1550,7 +1550,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>>   
>>   	rc = sev_read_init_ex_file();
>>   	if (rc)
>> -		return rc;
>> +		goto err_free;
>>   
>>   	/* If SEV-SNP is initialized, transition to firmware page. */
>>   	if (sev->snp_initialized) {
>> @@ -1559,11 +1559,23 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>>   		npages = 1UL << get_order(NV_LENGTH);
>>   		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>>   			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>> -			return -ENOMEM;
>> +			rc = -ENOMEM;
>> +			/*
>> +			 * Don't free on conversion failure: the rollback may
>> +			 * have left pages firmware-owned, and a high-order
>> +			 * block can't be partially freed.
>> +			 */
>> +			goto err_reset;
>>   		}
>>   	}
>>   
>>   	return 0;
>> +
>> +err_free:
>> +	__free_pages(page, get_order(NV_LENGTH));
>> +err_reset:
>> +	sev_init_ex_buffer = NULL;
>> +	return rc;
>>   }
>>   
>>   static int __sev_platform_init_locked(int *error)
>>

Re: [PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[Atish Patra]

On 6/2/26 7:38 AM, Tom Lendacky wrote:
> On 6/1/26 18:04, Atish Patra wrote:
>> From: Atish Patra <atishp@meta.com>
>>
>> The intra-host migration/mirroring feature is not fully implemented for
>> SEV-SNP VMs. The proper migration requires additional SNP-specific
>> state such as guest_req_mutex, guest_req_buf, and guest_resp_buf to be
>> transferred or initialized on the destination.
>>
>> The SNP VM mirroring requires vmsa features to be copied as well otherwise
>> ASID would be bound to SNP range while VM is detected as a SEV VM.
>>
>> Reject SNP source VMs in migration/mirroring until proper SNP state
>> transfer is implemented.
>>
>> Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
> Probably not the correct Fixes: tag. It should the tag that first
> introduces SNP hypervisor support.

Ahh yes. Fixed.

> And adding a comment above the if statements that indicate additional
> support is required for SNP, so don't allow it for now, would be nice.
Added.
> Otherwise, for the actual code...
>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
>> Reported-by: Chris Mason <clm@meta.com>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Assisted-by: Claude:claude-opus-4-6
>> Signed-off-by: Atish Patra <atishp@meta.com>
>> ---
>>   arch/x86/kvm/svm/sev.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index c2126b3c3072..e6ad6af128c9 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>>   		return ret;
>>   
>>   	if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
>> -	    sev_guest(kvm) || !sev_guest(source_kvm)) {
>> +	    sev_guest(kvm) || !sev_guest(source_kvm) ||
>> +	    sev_snp_guest(source_kvm)) {
>>   		ret = -EINVAL;
>>   		goto out_unlock;
>>   	}
>> @@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>>   	 * created after SEV/SEV-ES initialization, e.g. to init intercepts.
>>   	 */
>>   	if (sev_guest(kvm) || !sev_guest(source_kvm) ||
>> +	    sev_snp_guest(source_kvm) ||
>>   	    is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
>>   		ret = -EINVAL;
>>   		goto e_unlock;
>>

Re: [PATCH v2 3/4] crypto: ccp: Fix possible deadlock in SEV init failure path

[Atish Patra]

On 6/2/26 7:43 AM, Tom Lendacky wrote:
> On 6/1/26 18:04, Atish Patra wrote:
>> From: Atish Patra <atishp@meta.com>
>>
>> __sev_platform_init_handle_init_ex_path() called
> s/called/calls/
>
>> rmp_mark_pages_firmware() with locked=false but while the parent
> s/but//
>
>> function of init_ex_path already acquired the sev_cmd_mutex.
>> In case of a rmpupdate failure for any page after the first, the cleanup
> s/In case/In the case/
> s/a rmpupdate/an RMPUPDATE/
>
>> path would invoke reclaim pages which would result in a deadlock in
>> sev_do_cmd.
>>
>> Pass locked=true to honor the lock status of the parent function.
>>
>> Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
>>
>> Reported-by: Chris Mason <clm@meta.com>
>> Assisted-by: Claude:claude-opus-4-6
>> Signed-off-by: Atish Patra <atishp@meta.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
Thanks for the review. Fixed the typos in the commit text.

>> ---
>>   drivers/crypto/ccp/sev-dev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index d1e9e0ac63b6..3d4793e8e34b 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1557,7 +1557,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>>   		unsigned long npages;
>>   
>>   		npages = 1UL << get_order(NV_LENGTH);
>> -		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, false)) {
>> +		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>>   			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>>   			return -ENOMEM;
>>   		}
>>

Re: [PATCH v2 4/4] crypto: ccp: Fix memory leak in SEV INIT_EX path

[Atish Patra]

On 6/1/26 4:55 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] When SEV-SNP is initialized, `sev_init_ex_buffer` is explicitly transitioned to a firmware-owned state via `rmp_mark_pages_firmware()`. However, the driver never reclaims this buffer back to a shared state before reading it. Immediately after the `INIT_EX` command completes, `__sev_do_cmd_locked()` unconditionally calls `sev_write_init_ex_file_if_required()`, which invokes `kernel_write()` to read from `sev_init_ex_buffer` and save it to disk. The host CPU's attempt to read firmware-owned pages inside `kernel_write()` will trigger an RMP violation (Machine Check Exception or #PF with RMP bit), causing a fatal host crash.
> - [High] The `sev_init_ex_buffer` is allocated without the `__GFP_ZERO` flag. If the NV memory file does not exist (returning `-ENOENT`), or a short read occurs, `sev_read_init_ex_file()` returns 0 without fully initializing the 32KB buffer. This uninitialized kernel memory is passed to the SEV firmware via the `INIT_EX` command. Later, `sev_write_init_ex_file()` writes the entire 32KB buffer back to the host filesystem, resulting in an information leak of kernel heap data to disk.
> --
>
> commit 9089d07a571c9b175950b197e846ef7b16492229
> Author: Atish Patra <atishp@meta.com>
>
> crypto: ccp: Fix memory leak in SEV INIT_EX path
>
> This commit fixes a memory leak in the SEV INIT_EX path by freeing
> allocated pages and clearing the buffer on failure. It avoids freeing
> pages if they have already been converted to firmware-owned memory.
>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1550,7 +1550,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>>   
>>   	rc = sev_read_init_ex_file();
>>   	if (rc)
>> -		return rc;
>> +		goto err_free;
> [Severity: High]
> This is a pre-existing issue, but does allocating sev_init_ex_buffer
> without zeroing leak kernel memory?
>
> Earlier in __sev_platform_init_handle_init_ex_path(), the buffer is
> allocated without the zeroing flag:
>
> page = alloc_pages(GFP_KERNEL, get_order(NV_LENGTH));
>
> If the NV memory file does not exist, sev_read_init_ex_file() sets
> ret = 0 on -ENOENT and returns without initializing the buffer. Short
> reads also leave the remainder of the buffer uninitialized.
> Later, sev_write_init_ex_file() writes the entire 32KB buffer back to
> the host filesystem via kernel_write(). Could this result in an
> information leak of uninitialized kernel heap data to disk?

Fair point. I am not sure if the firmware implements writing to full 
NV_LENTH
so zeroing the buffer at allocation time is idea IMO.

>>   
>>   	/* If SEV-SNP is initialized, transition to firmware page. */
>>   	if (sev->snp_initialized) {
>> @@ -1559,11 +1559,23 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>>   		npages = 1UL << get_order(NV_LENGTH);
>>   		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>>   			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>> -			return -ENOMEM;
>> +			rc = -ENOMEM;
> [Severity: Critical]
> This is a pre-existing issue, but does reading a firmware-owned page
> from the host CPU trigger an RMP violation?
>
> When the buffer is explicitly transitioned to a firmware-owned state
> via rmp_mark_pages_firmware() here, the driver does not appear to
> reclaim this buffer back to a shared state before reading it.
>
> Immediately after the command completes, __sev_do_cmd_locked()
> unconditionally calls sev_write_init_ex_file_if_required(), which
> invokes kernel_write() to read from sev_init_ex_buffer and save it
> to disk.
>
> Will the host CPU's attempt to read firmware-owned pages inside
> kernel_write() trigger a fatal page fault or Machine Check Exception,
> crashing the host?

NA. Already invalidated the theory in response to v1.

https://lore.kernel.org/kvm/1dd5c998-ad2a-47dd-9875-c8fd5db6d374@linux.dev/

>
> [ ... ]
>

Re: [PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[Atish Patra]

On 6/1/26 4:17 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Missing `kvm->arch.vm_type` validation in `sev_vm_copy_enc_context_from` allows creating structurally inconsistent mirror VMs.
> - [High] Missing `vmsa_features` copy in `sev_vm_copy_enc_context_from` breaks SEV-ES mirror VMs that use features like `DEBUG_SWAP`.
> - [High] Concurrent list modification of `owner_sev_info->mirror_vms` without holding the owner VM's lock during mirror VM migration.
> --
>
> commit 5adb8a893221b0569310f07fd9fc7abca8088651
> Author: Atish Patra <atishp@meta.com>
>
> KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs
>
> This commit rejects SNP source VMs in migration/mirroring until proper SNP
> state transfer is implemented.
>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>>   		return ret;
>>   
>>   	if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
>> -	    sev_guest(kvm) || !sev_guest(source_kvm)) {
>> +	    sev_guest(kvm) || !sev_guest(source_kvm) ||
>> +	    sev_snp_guest(source_kvm)) {
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential locking race when
> an existing mirror VM is migrated via sev_vm_move_enc_context_from().
>
> Locks are only acquired for the source mirror (source_kvm) and destination
> VM (kvm):
>
> arch/x86/kvm/svm/sev.c:sev_vm_move_enc_context_from() {
>      ...
>      ret = sev_lock_two_vms(kvm, source_kvm);
>      if (ret)
>          return ret;
>      ...
> }
>
> Later, in sev_migrate_from(), it updates its owner's list of mirrors:
>
> arch/x86/kvm/svm/sev.c:sev_migrate_from() {
>      ...
>      if (is_mirroring_enc_context(dst_kvm)) {
>          struct kvm_sev_info *owner_sev_info =
>              to_kvm_sev_info(dst->enc_context_owner);
>
>          list_del(&src->mirror_entry);
>          list_add_tail(&dst->mirror_entry, &owner_sev_info->mirror_vms);
>      }
>      ...
> }
>
> Since the required lock for enc_context_owner is omitted entirely, could
> this concurrently modify the owner_sev_info->mirror_vms linked list if
> userspace creates a new mirror via sev_vm_copy_enc_context_from(), which
> locks the owner VM? Would this result in kernel list corruption?

It looks possible for SEV VMs looking at the code but I don't the know 
code path
or the mirroring/migration use case well enough to judge how likely this 
scenario
in real world.

>>   		ret = -EINVAL;
>>   		goto out_unlock;
>>   	}
>> @@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>>   	 * created after SEV/SEV-ES initialization, e.g. to init intercepts.
>>   	 */
>>   	if (sev_guest(kvm) || !sev_guest(source_kvm) ||
>> +	    sev_snp_guest(source_kvm) ||
>>   	    is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
> [Severity: High]
> This isn't a bug introduced by this patch, but this block seems to be
> missing the kvm->arch.vm_type != source_kvm->arch.vm_type check that
> exists in sev_vm_move_enc_context_from().
>
> Does this omission allow creating structurally inconsistent mirror VMs?
> Userspace could potentially create a destination VM as KVM_X86_SNP_VM and
> mirror a source VM of type KVM_X86_SEV_VM into it. Since KVM's core MMU
> would treat the new VM as an SNP VM (due to arch.vm_type), but the SEV
> code treats it as a legacy SEV VM (because ____sev_snp_guest evaluates
> to false), could this produce an inconsistent internal state and allow
> attaching guest_memfd to standard SEV VMs?
NA. They don't have to be same vm type as per my understanding from the 
selftest code.
>>   		ret = -EINVAL;
>>   		goto e_unlock;
> [Severity: High]
> This is a pre-existing issue, but I noticed another detail in
> sev_vm_copy_enc_context_from() just below this diff.
>
> The initialization block for the mirror VM omits copying vmsa_features:
>
> arch/x86/kvm/svm/sev.c:sev_vm_copy_enc_context_from() {
>      ...
>      mirror_sev->enc_context_owner = source_kvm;
>      mirror_sev->active = true;
>      mirror_sev->asid = source_sev->asid;
>      mirror_sev->fd = source_sev->fd;
>      mirror_sev->es_active = source_sev->es_active;
>      mirror_sev->need_init = false;
>      mirror_sev->handle = source_sev->handle;
>      ...
> }
>
> This omits mirror_sev->vmsa_features = source_sev->vmsa_features.
>
> Since SEV-ES VMs use vmsa_features to track capabilities like
> SVM_SEV_FEAT_DEBUG_SWAP, would a mirrored SEV-ES VM incorrectly
> initialize its vmsa_features to 0? When vCPUs are instantiated,
> sev_es_init_vmcb() checks sev_vcpu_has_debug_swap(), which would evaluate
> to false, resulting in misconfigured VMCB intercepts. Could this silently
> drop important execution features for SEV-ES mirrors?

As per my understanding, that is intentional as well.
Is it common to mirror SEV VMs with SVM_SEV_FEAT_DEBUG_SWAP enabled ?

@tom: Please let me know if any of the above statements are not true.

Re: [PATCH v2 3/4] crypto: ccp: Fix possible deadlock in SEV init failure path

[Atish Patra]

On 6/1/26 4:41 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Failure in snp_prep_cmd_buf permanently leaks the command buffer active state in __sev_do_cmd_locked, deadlocking the SEV interface.
> - [High] sev_init_ex_buffer is allocated without zeroing, leaking uninitialized kernel memory to the host filesystem.
> - [High] Unhandled allocation failure for dev_data->output in spdm_ctrl_alloc leads to passing a null physical address to the PSP firmware.
> - [High] Retrying SEV-TIO commands after buffer expansion uses stale, freed physical addresses, leading to RMP violations.
> - [High] Bounce buffer memory is permanently leaked when firmware page transitions fail during legacy SEV command preparation.
> --
>
> commit 1396d09c5ba264e4a091f5fa101bb062c384525c
> Author: Atish Patra <atishp@meta.com>
>
> crypto: ccp: Fix possible deadlock in SEV init failure path
>
> __sev_platform_init_handle_init_ex_path() called
> rmp_mark_pages_firmware() with locked=false but while the parent
> function of init_ex_path already acquired the sev_cmd_mutex.
> In case of a rmpupdate failure for any page after the first, the cleanup
> path would invoke reclaim pages which would result in a deadlock in
> sev_do_cmd.
>
> Pass locked=true to honor the lock status of the parent function.
>
> Fixes: 7364a6fbca45 ("crypto: ccp: Handle non-volatile INIT_EX data when SNP is enabled")
> Reported-by: Chris Mason <clm@meta.com>
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Atish Patra <atishp@meta.com>
>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index d1e9e0ac63b60..3d4793e8e34b2 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1557,7 +1557,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>>   		unsigned long npages;
>>   
>>   		npages = 1UL << get_order(NV_LENGTH);
>> -		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, false)) {
>> +		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>>   			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>>   			return -ENOMEM;
>>   		}
> [Severity: High]
> This is a pre-existing issue, but does this allocation leak uninitialized
> memory?
>
> Looking at __sev_platform_init_handle_init_ex_path() in the same file:
>
> drivers/crypto/ccp/sev-dev.c:__sev_platform_init_handle_init_ex_path() {
>      ...
>      page = alloc_pages(GFP_KERNEL, get_order(NV_LENGTH));
>      ...
> }
>
> If the NV file does not exist, sev_read_init_ex_file() returns 0 without
> writing any data. This leaves the 32KB buffer filled with uninitialized
> kernel memory before transitioning to firmware state. When
> sev_write_init_ex_file() later writes this buffer to the host filesystem,
> could this persist kernel heap contents to disk if the firmware did not
> fully overwrite it?
fixed in PATCH 4.
> [Severity: High]
> This is a pre-existing issue, but can this error path leave the command
> buffer permanently active?
>
> drivers/crypto/ccp/sev-dev.c:__sev_do_cmd_locked() {
>      ...
>      ret = snp_prep_cmd_buf(cmd, cmd_buf, desc_list);
>      if (ret) {
>          dev_err(sev->dev, "SEV: failed to prepare buffer...");
>          return ret;
>      }
>      ...
> }
>
> Since sev->cmd_buf_active or sev->cmd_buf_backup_active were set to true
> before this call, returning directly here bypasses resetting those flags.
> Would this cause subsequent commands to fail with -EBUSY and lock up the
> interface?
>
> [Severity: High]
> This is a pre-existing issue, but does this error path leak the mapped
> bounce buffers?
>
> drivers/crypto/ccp/sev-dev.c:snp_prep_cmd_buf() {
>      ...
>      if (sev_cmd_buf_writable(cmd)) {
>          if (rmp_mark_pages_firmware(__pa(cmd_buf), 1, true))
>              return -EFAULT;
>      }
>      ...
> }
>
> If this fails, it returns -EFAULT without calling
> snp_unmap_cmd_buf_desc_list(), which appears to leave the mapped command
> descriptors unmapped.
>
> Similarly, in snp_map_cmd_buf_desc():
>
> drivers/crypto/ccp/sev-dev.c:snp_map_cmd_buf_desc() {
>      ...
>      if (rmp_mark_pages_firmware(*desc->paddr_ptr, npages, true)) {
>          pr_warn("Error moving pages to firmware-owned state...");
>          return -EFAULT;
>      }
>      ...
> }
>
> If the firmware transition fails for an individual bounce buffer, the newly
> allocated page isn't freed before returning -EFAULT. Could both of these
> paths result in permanent memory leaks?
>
> [Severity: High]
> This is a pre-existing issue, but does this validation correctly check the
> output buffer?
>
> drivers/crypto/ccp/sev-dev-tio.c:spdm_ctrl_alloc() {
>      ...
>      dev_data->output = sla_alloc(dev_data->output_len, true);
>
>      if (IS_SLA_NULL(dev_data->req) || IS_SLA_NULL(dev_data->resp) ||
>          IS_SLA_NULL(dev_data->scratch) || IS_SLA_NULL(dev_data->dev_ctx)) {
>      ...
> }
>
> The check examines dev_ctx instead of the newly allocated output. If the
> output allocation fails, dev_data->output will be SLA_NULL (physical frame
> number 0). Will returning success here cause the PSP firmware to use
> physical page 0 as an output buffer and trigger an RMP violation?
>
> [Severity: High]
> This is a pre-existing issue, but does retrying the command use stale
> physical addresses?
>
> drivers/crypto/ccp/sev-dev-tio.c:sev_tio_do_cmd() {
>      ...
>      rc1 = sla_expand(&dev_data->output, &dev_data->output_len);
>      ...
>      rc = sev_do_cmd(cmd, data, psp_ret);
>      ...
> }
>
> When sla_expand() allocates a new buffer and frees the old one, dev_data
> pointers are updated. However, the data pointer passed to sev_do_cmd()
> comes from the caller and contains a by-value copy of the old spdm_ctrl
> fields. Will retrying the command pass the old, freed physical addresses
> to the PSP firmware?
>
Above issues require a bit detailed investigation to see if it is a 
false positive.
These are completely unrelated to this patch as well.
Does these needs to be addressed before merging ?

Re: [PATCH v2 1/4] KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs

[Tom Lendacky]

On 6/2/26 16:55, Atish Patra wrote:
> 
> On 6/1/26 4:17 PM, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential
>> issue(s) to consider:

Adding @Peter Gonda who knows this code better than I do!

Thanks,
Tom
>>
>> Pre-existing issues:
>> - [High] Missing `kvm->arch.vm_type` validation in
>> `sev_vm_copy_enc_context_from` allows creating structurally
>> inconsistent mirror VMs.
>> - [High] Missing `vmsa_features` copy in
>> `sev_vm_copy_enc_context_from` breaks SEV-ES mirror VMs that use
>> features like `DEBUG_SWAP`.
>> - [High] Concurrent list modification of `owner_sev_info->mirror_vms`
>> without holding the owner VM's lock during mirror VM migration.
>> -- 
>>
>> commit 5adb8a893221b0569310f07fd9fc7abca8088651
>> Author: Atish Patra <atishp@meta.com>
>>
>> KVM: SEV: Do not allow intra-host migration/mirroring of SNP VMs
>>
>> This commit rejects SNP source VMs in migration/mirroring until proper
>> SNP
>> state transfer is implemented.
>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -2142,7 +2142,8 @@ int sev_vm_move_enc_context_from(struct kvm
>>> *kvm, unsigned int source_fd)
>>>           return ret;
>>>         if (kvm->arch.vm_type != source_kvm->arch.vm_type ||
>>> -        sev_guest(kvm) || !sev_guest(source_kvm)) {
>>> +        sev_guest(kvm) || !sev_guest(source_kvm) ||
>>> +        sev_snp_guest(source_kvm)) {
>> [Severity: High]
>> This is a pre-existing issue, but I noticed a potential locking race when
>> an existing mirror VM is migrated via sev_vm_move_enc_context_from().
>>
>> Locks are only acquired for the source mirror (source_kvm) and
>> destination
>> VM (kvm):
>>
>> arch/x86/kvm/svm/sev.c:sev_vm_move_enc_context_from() {
>>      ...
>>      ret = sev_lock_two_vms(kvm, source_kvm);
>>      if (ret)
>>          return ret;
>>      ...
>> }
>>
>> Later, in sev_migrate_from(), it updates its owner's list of mirrors:
>>
>> arch/x86/kvm/svm/sev.c:sev_migrate_from() {
>>      ...
>>      if (is_mirroring_enc_context(dst_kvm)) {
>>          struct kvm_sev_info *owner_sev_info =
>>              to_kvm_sev_info(dst->enc_context_owner);
>>
>>          list_del(&src->mirror_entry);
>>          list_add_tail(&dst->mirror_entry, &owner_sev_info->mirror_vms);
>>      }
>>      ...
>> }
>>
>> Since the required lock for enc_context_owner is omitted entirely, could
>> this concurrently modify the owner_sev_info->mirror_vms linked list if
>> userspace creates a new mirror via sev_vm_copy_enc_context_from(), which
>> locks the owner VM? Would this result in kernel list corruption?
> 
> It looks possible for SEV VMs looking at the code but I don't the know
> code path
> or the mirroring/migration use case well enough to judge how likely this
> scenario
> in real world.
> 
>>>           ret = -EINVAL;
>>>           goto out_unlock;
>>>       }
>>> @@ -2865,6 +2866,7 @@ int sev_vm_copy_enc_context_from(struct kvm
>>> *kvm, unsigned int source_fd)
>>>        * created after SEV/SEV-ES initialization, e.g. to init
>>> intercepts.
>>>        */
>>>       if (sev_guest(kvm) || !sev_guest(source_kvm) ||
>>> +        sev_snp_guest(source_kvm) ||
>>>           is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
>> [Severity: High]
>> This isn't a bug introduced by this patch, but this block seems to be
>> missing the kvm->arch.vm_type != source_kvm->arch.vm_type check that
>> exists in sev_vm_move_enc_context_from().
>>
>> Does this omission allow creating structurally inconsistent mirror VMs?
>> Userspace could potentially create a destination VM as KVM_X86_SNP_VM and
>> mirror a source VM of type KVM_X86_SEV_VM into it. Since KVM's core MMU
>> would treat the new VM as an SNP VM (due to arch.vm_type), but the SEV
>> code treats it as a legacy SEV VM (because ____sev_snp_guest evaluates
>> to false), could this produce an inconsistent internal state and allow
>> attaching guest_memfd to standard SEV VMs?
> NA. They don't have to be same vm type as per my understanding from the
> selftest code.
>>>           ret = -EINVAL;
>>>           goto e_unlock;
>> [Severity: High]
>> This is a pre-existing issue, but I noticed another detail in
>> sev_vm_copy_enc_context_from() just below this diff.
>>
>> The initialization block for the mirror VM omits copying vmsa_features:
>>
>> arch/x86/kvm/svm/sev.c:sev_vm_copy_enc_context_from() {
>>      ...
>>      mirror_sev->enc_context_owner = source_kvm;
>>      mirror_sev->active = true;
>>      mirror_sev->asid = source_sev->asid;
>>      mirror_sev->fd = source_sev->fd;
>>      mirror_sev->es_active = source_sev->es_active;
>>      mirror_sev->need_init = false;
>>      mirror_sev->handle = source_sev->handle;
>>      ...
>> }
>>
>> This omits mirror_sev->vmsa_features = source_sev->vmsa_features.
>>
>> Since SEV-ES VMs use vmsa_features to track capabilities like
>> SVM_SEV_FEAT_DEBUG_SWAP, would a mirrored SEV-ES VM incorrectly
>> initialize its vmsa_features to 0? When vCPUs are instantiated,
>> sev_es_init_vmcb() checks sev_vcpu_has_debug_swap(), which would evaluate
>> to false, resulting in misconfigured VMCB intercepts. Could this silently
>> drop important execution features for SEV-ES mirrors?
> 
> As per my understanding, that is intentional as well.
> Is it common to mirror SEV VMs with SVM_SEV_FEAT_DEBUG_SWAP enabled ?
> 
> @tom: Please let me know if any of the above statements are not true.
> 
>