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

On 7/27/2026 2:22 PM, [email protected] wrote:
> 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 <[email protected]>
> 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?

Can't happen in the full series: patch 4 adds if (!rmpopt_wq) return;, and rmpopt_wq is non-NULL only after snp_setup_rmpopt() succeeds.
On a host without RMPOPT it stays NULL, so rmpopt_cpumask is never dereferenced.

> 
>> +		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.

Handled in the full series: patch 4's re-init guard (if (rmpopt_wq) { queue_delayed_work(...); return; }) re-queues the pass instead of re-allocating,
so there's no leak.

> 
>> +		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.

Intentional. snp_prepare() re-enables hotplug on its own failure (before SnpEn is set); once SnpEn is set, hotplug must stay disabled — 
a CPU coming online without it would break RMP-check consistency and any SNP_INIT retry (which needs SnpEn on all present CPUs). 
Documented in the commit message, and the triggering failure is already logged.

Thanks,
Ashish

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