linux-kernel - Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 9 Apr 2024 12:37:14 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: "Yamahata, Isaku" <isaku.yamahata@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>, Paolo Bonzini
	<pbonzini@...hat.com>, "Aktas, Erdem" <erdemaktas@...gle.com>, "Sean
 Christopherson" <seanjc@...gle.com>, Sagi Shahar <sagis@...gle.com>, "Chen,
 Bo2" <chen.bo@...el.com>, "Yuan, Hang" <hang.yuan@...el.com>, "Zhang, Tina"
	<tina.zhang@...el.com>
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module



On 26/02/2024 9:25 pm, Yamahata, Isaku wrote:
> +struct tdx_enabled {
> +	cpumask_var_t enabled;
> +	atomic_t err;
> +};
> +
> +static void __init tdx_on(void *_enable)
> +{
> +	struct tdx_enabled *enable = _enable;
> +	int r;
> +
> +	r = vmx_hardware_enable();
> +	if (!r) {
> +		cpumask_set_cpu(smp_processor_id(), enable->enabled);
> +		r = tdx_cpu_enable();
> +	}
> +	if (r)
> +		atomic_set(&enable->err, r);
> +}
> +
> +static void __init vmx_off(void *_enabled)
> +{
> +	cpumask_var_t *enabled = (cpumask_var_t *)_enabled;
> +
> +	if (cpumask_test_cpu(smp_processor_id(), *enabled))
> +		vmx_hardware_disable();
> +}
> +
> +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> +{
> +	struct tdx_enabled enable = {
> +		.err = ATOMIC_INIT(0),
> +	};
> +	int r = 0;
> +
> +	if (!enable_ept) {
> +		pr_warn("Cannot enable TDX with EPT disabled\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!zalloc_cpumask_var(&enable.enabled, GFP_KERNEL)) {
> +		r = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* tdx_enable() in tdx_module_setup() requires cpus lock. */
> +	cpus_read_lock();
> +	on_each_cpu(tdx_on, &enable, true); /* TDX requires vmxon. */
> +	r = atomic_read(&enable.err);
> +	if (!r)
> +		r = tdx_module_setup();
> +	else
> +		r = -EIO;

I was thinking why do we need to convert to -EIO.

Convert error code to -EIO unconditionally would cause the original 
error code being lost.  Although given tdx_on() is called on all online 
cpus in parallel, the @enable.err could be imprecise anyway, the 
explicit conversion seems not quite reasonable to be done _here_.

I think it would be more reasonable to explicitly set the error code to 
-EIO in tdx_on(), where we _exactly_ know what went wrong and can still 
possibly do something before losing the error code.

E.g., we can dump the error code to the user, but looks both 
vmx_hardware_enable() and tdx_cpu_enable() will do so already so we can 
safely lose the error code there.

We can perhaps add a comment to point this out before losing the error 
code if that's better:

	/*
	 * Both vmx_hardware_enable() and tdx_cpu_enable() print error
	 * message when they fail.  Just convert the error code to -EIO
	 * when multiple cpu fault the @err cannot be used to precisely
	 * record the error code for them anyway.
	 */

> +	on_each_cpu(vmx_off, &enable.enabled, true);
> +	cpus_read_unlock();
> +	free_cpumask_var(enable.enabled);
> +
> +out:
> +	return r;
> +}