Re: [PATCH v11 3/6] x86/sev: Initialize RMPOPT configuration MSRs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `snp_cleanup_rmpopt()` unconditionally dereferences `rmpopt_cpumask`, causing a deterministic kernel panic on shutdown if the mask was never allocated (e.g., on CPUs without RMPOPT support or if allocation fails).
- [High] If SEV-SNP firmware initialization fails, the error path returns directly without cleaning up the state set by `snp_prepare()`, leaving CPU hotplug permanently disabled.
- [Medium] Re-initializing SEV-SNP after a legacy shutdown leaks the `rmpopt_cpumask` allocation because the old pointer is overwritten without being freed.
--

--- Patch [3]: [PATCH v11 3/6] x86/sev: Initialize RMPOPT configuration MSRs ---
commit e4623d49ab6201adc5b609f5a6043ed6dfb7f55c
Author: Ashish Kalra <ashish.kalra@amd.com>
Subject: x86/sev: Initialize RMPOPT configuration MSRs

The new RMPOPT instruction helps manage per-CPU RMP optimization
structures inside the CPU. This commit initializes the per-CPU RMPOPT
table base to enable RMP optimization for up to 2 TB of system RAM.

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index e2f69fba09389..8bfd802848361 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
[ ... ]
> @@ -558,6 +561,17 @@ int snp_prepare(void)
>  }
>  EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
>  
> +static void snp_cleanup_rmpopt(void)
> +{
> +	int cpu;
> +
> +	for_each_cpu(cpu, rmpopt_cpumask)

[Severity: High]
If rmpopt_cpumask was never allocated, does this macro dereference a NULL
pointer?

In snp_setup_rmpopt(), if rmpopt_capable() is false, the mask remains
unallocated. When snp_shutdown() subsequently calls snp_cleanup_rmpopt(),
it iterates over the mask without checking if it is available.

Can this cause a kernel panic on system shutdown or module unload for
systems without RMPOPT support?

> +		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, 0);
> +
> +	free_cpumask_var(rmpopt_cpumask);
> +	rmpopt_pa_start = 0;
> +}
[ ... ]
> @@ -578,6 +593,46 @@ void snp_shutdown(void)
>  }
>  EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
>  
> +static bool rmpopt_capable(void)
> +{
> +	return cpu_feature_enabled(X86_FEATURE_RMPOPT) &&
> +	       cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> +}
> +
> +void snp_setup_rmpopt(void)
> +{
> +	u64 rmpopt_base;
> +	int cpu;
> +
> +	if (!rmpopt_capable())
> +		return;
> +
> +	if (!zalloc_cpumask_var(&rmpopt_cpumask, GFP_KERNEL)) {

[Severity: Medium]
Does this unconditional allocation leak the previously allocated cpumask
when SEV-SNP is re-initialized?

During a legacy shutdown, snp_shutdown() returns early when the SnpEn
firmware bit is still set, skipping snp_cleanup_rmpopt():

arch/x86/virt/svm/sev.c:snp_shutdown() {
...
	rdmsrq(MSR_AMD64_SYSCFG, syscfg);
	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
		return;
...
}

A subsequent call to __sev_snp_init_locked() will invoke snp_setup_rmpopt()
again and overwrite the old pointer without freeing it.

> +		pr_err("Failed to allocate RMPOPT cpumask\n");
> +		return;
> +	}
[ ... ]
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index f833cb7e4da36..0b40d23a26b45 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1630,6 +1630,9 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  	}
>  
>  	snp_hv_fixed_pages_state_update(sev, HV_FIXED);
> +
> +	snp_setup_rmpopt();
> +

[Severity: High]
If the SEV-SNP firmware initialization fails, does this leave CPU hotplug
permanently disabled?

A global cpu_hotplug_disable() call is made inside snp_prepare() right before
we reach this code. If the subsequent firmware command fails in
__sev_snp_init_locked(), it returns the error directly without cleaning up:

drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked() {
...
	rc = __sev_do_cmd_locked(cmd, arg, error);
	if (rc) {
		dev_err(sev->dev, "SEV-SNP: %s failed rc %d, error %#x\n",
			cmd == SEV_CMD_SNP_INIT_EX ? "SNP_INIT_EX" : "SNP_INIT",
			rc, *error);
		return rc;
	}
...
}

Since sev->snp_initialized remains false on this error path, snp_shutdown()
is never executed on teardown, meaning cpu_hotplug_enable() is never invoked
to release the lock.

>  	sev->snp_initialized = true;
>  	dev_dbg(sev->dev, "SEV-SNP firmware initialized, SEV-TIO is %s\n",
>  		data.tio_en ? "enabled" : "disabled");

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