Re: [PATCH v13 2/5] x86/sev: 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:
- [Critical] PM Suspend (S3) bypasses CPU hotplug disable, leading to security breakdown
- [High] CPU hotplug remains enabled on kexec boot if setup_rmptable() fails
--

commit c91487bb659a8ce73e581c086c8aa1aa19f4b9aa
Author: Ashish Kalra <ashish.kalra@amd.com>
x86/sev: Disable CPU hotplug while SNP is active

This patch disables CPU hotplug while SNP is active to ensure no CPU comes
online without the SnpEn bit set. It ties hotplug to the SNP-enable bit by
disabling it in snp_prepare() and re-enabling it in snp_shutdown().

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index cff285d8ad8e3..558f7924a3f85 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c

[ ... ]

> @@ -526,14 +525,18 @@ int snp_prepare(void)
>  
>  	clear_rmp();
>  
> -	cpus_read_lock();
> +	/*
> +	 * No CPU may come online without SnpEn while SNP is active; disable
> +	 * hotplug here and re-enable it in snp_shutdown().
> +	 */
> +	cpu_hotplug_disable();

[Severity: Critical]
Does this sequence prevent CPUs from being onlined without SnpEn set during
power management transitions?

While cpu_hotplug_disable() blocks sysfs-initiated hotplug operations, it
appears that system suspend (S3) and hibernation (S4) bypass this mechanism.
During suspend, the PM core uses freeze_secondary_cpus() and
thaw_secondary_cpus(), which explicitly call _cpu_down() and _cpu_up()
regardless of the cpu_hotplug_disabled counter.

Additionally, MSR_AMD64_SYSCFG is not included in the list of MSRs saved and
restored during PM transitions in arch/x86/power/cpu.c.

If a system with SNP active goes to sleep and resumes, will the secondary
CPUs wake up without the SnpEn bit set, potentially breaking the SEV-SNP
security model by failing to enforce RMP checks?

[ ... ]

> @@ -577,6 +582,8 @@ EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
>   */
>  int __init snp_rmptable_init(void)
>  {
> +	u64 val;
> +
>  	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
>  		return -ENOSYS;
>  
> @@ -586,6 +593,15 @@ int __init snp_rmptable_init(void)
>  	if (!setup_rmptable())
>  		return -ENOSYS;

[Severity: High]
If setup_rmptable() fails and returns -ENOSYS early, do we leave CPU hotplug
enabled even though the hardware might still have SNP enabled from a previous
kernel?

When booting via kexec from an SNP-enabled kernel, the CPUs retain the SnpEn
bit in MSR_AMD64_SYSCFG. If setup_rmptable() fails here (for example, due to
a memremap failure), the function exits before reaching the new check that
disables CPU hotplug.

Because the function failed, the kernel clears CC_ATTR_HOST_SEV_SNP. If a CPU
is subsequently hotplugged via sysfs, the new kernel will not set SnpEn on
it, resulting in a state where boot CPUs have SnpEn set but the hotplugged
CPU does not.

Could this early return be moved after the MSR_AMD64_SYSCFG_SNP_EN check to
ensure hotplug is disabled whenever the hardware indicates SNP is active?

> +
> +	/*
> +	 * On a kexec boot SNP may already be enabled (legacy firmware leaves
> +	 * SnpEn set across shutdown), in which case snp_prepare() bails without
> +	 * disabling CPU hotplug, so disable it here.
> +	 */
> +	rdmsrq(MSR_AMD64_SYSCFG, val);
> +	if (val & MSR_AMD64_SYSCFG_SNP_EN)
> +		cpu_hotplug_disable();
> +
>  	/*
>  	 * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic
>  	 * notifier is invoked to do SNP IOMMU shutdown before kdump.

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