Re: [PATCH v4 0/7] Move SNP initialization to the CCP driver

[Tycho Andersen <tycho@kernel.org>]

On Wed, Mar 25, 2026 at 09:07:42AM +0000, Borislav Petkov wrote:
> Sachiko has some questions:
> 
> https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org

Interesting, review-prompts directly didn't complain about these,

> Could this lead to a NULL pointer dereference in clear_rmp() if
> setup_rmptable() fails during early boot?
>
> If setup_rmptable() returns false (for example, if the BIOS did not
> reserve memory for the RMP table), it exits without allocating
> rmp_bookkeeping or rmp_segment_table, but leaves the CC_ATTR_HOST_SEV_SNP
> platform attribute active.

If setup_rmptable() fails, snp_rmptable_init() returns -ENOSYS and
iommu_snp_enable() clears CC_ATTR_HOST_SEV_SNP.
__sev_snp_init_locked() checks CC_ATTR_HOST_SEV_SNP before doing
anything else, so I think this is not possible.

You did complain about this call chain before off-list though, maybe
we should clear CC_ATTR_HOST_SEV_SNP in more places directly vs.
returning an errno to make it more obvious?

> Is there a race condition with CPU hotplug here?
>
> Since snp_prepare() lacks cpus_read_lock() protection, a CPU could
> come online exactly between the two passes, missing the mfd_enable step
> but receiving snp_enable.

I think it makes sense to do the operations on the same set of CPUs
even if we don't support hotplug. I will resend with
cpus_read_lock().

> Additionally, without a CPU hotplug state callback (such as
> cpuhp_setup_state()), any CPUs brought online after snp_prepare()
> completes will miss these MSR configurations completely. If an SNP
> guest is later scheduled on one of these uninitialized CPUs, will it cause
> hardware exceptions?

Yes, WONTFIX.

> Can deferring this call cause a NULL pointer dereference? Previously,
> snp_prepare() was called only after setup_rmptable() succeeded. By
> moving it to the CCP driver, it may be called unconditionally as long
> as CC_ATTR_HOST_SEV_SNP is true, even if setup_rmptable() was skipped
> or failed. Does clear_rmp() inside snp_prepare() safely handle the
> uninitialized rmp_bookkeeping pointer?

Same as above, CC_ATTR_HOST_SEV_SNP should perhaps be cleared in more
places.

> Also, moving snp_prepare() out of early boot might expand the window for
> CPU hotplug events. Does this create an asymmetry where newly onlined
> CPUs miss the MSR_AMD64_SYSCFG_SNP_EN configuration applied by
> on_each_cpu(snp_enable, NULL, 1) because the SEV subsystem lacks a CPU
> hotplug callback? Could this cause host crashes when KVM schedules VMs
> on these uninitialized CPUs?

Same as above, WONTFIX.

> Additionally, does deferring SNP enablement in the hardware coordinate
> with the IOMMU feature degradation? The AMD IOMMU driver evaluates
> amd_iommu_snp_en at early boot and permanently disables identity mapping
> domains and IOMMUv2. Will the system suffer this permanent penalty even
> if the CCP driver is never loaded?

This is about the bios setting:

        amd_iommu_snp_en = check_feature(FEATURE_SNP);
        if (!amd_iommu_snp_en) {
                pr_warn("SNP: IOMMU SNP feature not enabled, SNP cannot be supported.\n");
                goto disable_snp;
        }

so I don't think the question really makes sense.

> Could placing snp_prepare() here cause prolonged blocking of all SEV
> operations? Since __sev_snp_init_locked() holds sev_cmd_mutex, the long
> execution time of clear_rmp() (which zeroes the entire RMP table and scales
> with system RAM) might block SEV firmware ioctls and VM lifecycle operations
> globally for several seconds.

Yes, it does :). Actually the firmware call is the expensive part, not
the rmp zeroing, but it definitely blocks for a while.

> This isn't a bug introduced by this commit, but 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?

Oof, yes. I wonder if we shouldn't set psp_dead = true if
sev_platform_init() sees an error. After this series, if
the error is e.g. init_ex_path failure, you can unload, fix the
failure, and try again.

>  	if (sev_version_greater_or_equal(SNP_MIN_API_MAJOR, 52)) {
[ ... ]
>  		memset(&data, 0, sizeof(data));
[ ... ]
>  		data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();
[ ... ]
>  	} else {
>  		cmd = SEV_CMD_SNP_INIT;
>  		arg = NULL;
>  	}
> This isn't a bug introduced by this commit, but is the stack variable
> data left uninitialized when taking the else branch? Since data.tio_en is
> later evaluated unconditionally, could stack garbage cause it to evaluate
> to true, leading to erroneous attempts to allocate pages and initialize
> SEV-TIO on unsupported hardware?

No, arg is the actual pointer passed, and it is set to NULL. non-EX
init doesn't support TIO anyway...

> Also, regarding the bounds check in snp_filter_reserved_mem_regions()
> called via walk_iomem_res_desc(): does the check
> if ((range_list->num_elements * 16 + 8) > PAGE_SIZE)
> allow an off-by-one heap buffer overflow?
>
> If range_list->num_elements is 255, 255 * 16 + 8 = 4088, which is <= 4096.
> Writing range->base (8 bytes) fills 4088-4095, but writing range->page_count
> (4 bytes) would write to 4096-4099, overflowing the kzalloc-allocated
> PAGE_SIZE buffer.

Yes, this also looks real, and needs:

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 939fa8aa155c..3642226c0fc0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1328,10 +1328,11 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 	size_t size;
 
 	/*
-	 * Ensure the list of HV_FIXED pages that will be passed to firmware
-	 * do not exceed the page-sized argument buffer.
+	 * Ensure the list of HV_FIXED pages including the one we are about to
+	 * use that will be passed to firmware do not exceed the page-sized
+	 * argument buffer.
 	 */
-	if ((range_list->num_elements * sizeof(struct sev_data_range) +
+	if (((range_list->num_elements + 1) * sizeof(struct sev_data_range) +
 	     sizeof(struct sev_data_range_list)) > PAGE_SIZE)
 		return -E2BIG;

I have another bug that review-prompts found unrelated to this series.
I can put the two fixes above with that or include them here, let me
know what you prefer. Either way I'll resend one more with
cpus_read_lock().

Thanks,

Tycho