Re: [PATCH v8 3/7] crypto/ccp: Disable CPU hotplug while SNP is active

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `snp_cpu_hotplug_disabled` flag is a module-local static variable used to track the global `cpu_hotplug_disabled` counter. On systems with legacy firmware, CPU hotplug is intentionally left disabled when shutting down SNP. However, unloading and reloading the `ccp` module resets this static flag, causing subsequent SNP initializations to call `cpu_hotplug_disable()` again. This unintentionally stacks the global counter, leading to a reference leak over multiple load/unload cycles.

Pre-existing issues:
- [High] If `SEV_CMD_SNP_DF_FLUSH` fails during SNP initialization, the firmware state of HV_FIXED pages is leaked, potentially leading to RMP faults and host crashes when the pages are freed.
--

commit f12f8db5e522836adb88644cf2b682abf1545290
Author: Ashish Kalra <ashish.kalra@amd.com>

crypto/ccp: Disable CPU hotplug while SNP is active

This commit disables CPU hotplug during SNP execution to prevent
divergence between OS and firmware CPU expectations. It adds a static
flag to balance enable/disable calls and avoids re-enabling hotplug
on the legacy shutdown path.

> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index c002a7ca26a8a..cdda30a7677e2 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c

[ ... ]

> @@ -1478,6 +1481,17 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  
>  	snp_hv_fixed_pages_state_update(sev, HV_FIXED);

[Severity: High]
This isn't a bug introduced by this patch, but if the preceding
SEV_CMD_SNP_DF_FLUSH command fails, the function returns early and
bypasses updating the software state of the HV_FIXED pages.

If the caller handles the initialization failure and cleans up via
snp_free_hv_fixed_pages(), will it see the state as ALLOCATED instead
of HV_FIXED?

drivers/crypto/ccp/sev-dev.c:snp_free_hv_fixed_pages() {
	...
	if (entry->page_state == HV_FIXED) {
		entry->free = true;
	} else {
		__free_pages(page, entry->order);
		list_del(&entry->list);
		kfree(entry);
	}
	...
}

Could this incorrectly free firmware-locked pages to the buddy allocator,
potentially leading to RMP violations and host crashes later when those
pages are accessed?

>  
> +	/*
> +	 * Disable CPU hotplug while SNP is active.  Guard against stacking
> +	 * the disable count: the legacy SNP_SHUTDOWN_EX path clears
> +	 * snp_initialized without re-enabling hotplug, so this can run
> +	 * again while hotplug is already disabled.
> +	 */
> +	if (!snp_cpu_hotplug_disabled) {
> +		cpu_hotplug_disable();
> +		snp_cpu_hotplug_disabled = true;
> +	}
> +
>  	snp_setup_rmpopt();

[Severity: Medium]
Since snp_cpu_hotplug_disabled is a module-local static variable,
does it get destroyed and reset to false when the ccp module is
unloaded?

If the legacy shutdown path intentionally keeps hotplug disabled,
would unloading and reloading the module cause this code to evaluate to
true?

If so, won't cpu_hotplug_disable() be called again, stacking the global
counter and causing a reference leak over multiple load/unload cycles?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781419998.git.ashish.kalra@amd.com?part=3