| 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
| ||
|
Message-ID: <20230605204636.GA3346834@private.email.ne.jp>
Date: Mon, 5 Jun 2023 13:46:36 -0700
From: Isaku Yamahata <isaku.yamahata@...il.com>
To: Zhi Wang <zhi.wang.linux@...il.com>
Cc: Isaku Yamahata <isaku.yamahata@...il.com>,
isaku.yamahata@...el.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
erdemaktas@...gle.com, Sean Christopherson <seanjc@...gle.com>,
Sagi Shahar <sagis@...gle.com>,
David Matlack <dmatlack@...gle.com>,
Kai Huang <kai.huang@...el.com>, chen.bo@...el.com
Subject: Re: [PATCH v14 111/113] RFC: KVM: x86, TDX: Add check for setting
CPUID
On Mon, Jun 05, 2023 at 10:25:11AM +0800,
Zhi Wang <zhi.wang.linux@...il.com> wrote:
> > > > +
> > > > void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > {
> > > > struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > @@ -2003,10 +2044,12 @@ static void setup_tdparams_eptp_controls(struct kvm_cpuid2 *cpuid, struct td_par
> > > > }
> > > > }
> > > >
> > > > -static void setup_tdparams_cpuids(const struct tdsysinfo_struct *tdsysinfo,
> > > > +static void setup_tdparams_cpuids(struct kvm *kvm,
> > > > + const struct tdsysinfo_struct *tdsysinfo,
> > > > struct kvm_cpuid2 *cpuid,
> > > > struct td_params *td_params)
> > > > {
> > > > + struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > > > int i;
> > > >
> > > > /*
> > > > @@ -2014,6 +2057,7 @@ static void setup_tdparams_cpuids(const struct tdsysinfo_struct *tdsysinfo,
> > > > * be same to the one of struct tdsysinfo.{num_cpuid_config, cpuid_configs}
> > > > * It's assumed that td_params was zeroed.
> > > > */
> > > > + kvm_tdx->cpuid_nent = 0;
> > > > for (i = 0; i < tdsysinfo->num_cpuid_config; i++) {
> > > > const struct tdx_cpuid_config *config = &tdsysinfo->cpuid_configs[i];
> > > > /* TDX_CPUID_NO_SUBLEAF in TDX CPUID_CONFIG means index = 0. */
> > > > @@ -2035,6 +2079,10 @@ static void setup_tdparams_cpuids(const struct tdsysinfo_struct *tdsysinfo,
> > > > value->ebx = entry->ebx & config->ebx;
> > > > value->ecx = entry->ecx & config->ecx;
> > > > value->edx = entry->edx & config->edx;
> > > > +
> > > > + /* Remember the setting to check for KVM_SET_CPUID2. */
> > > > + kvm_tdx->cpuid[kvm_tdx->cpuid_nent] = *entry;
> > > > + kvm_tdx->cpuid_nent++;
> > > > }
> > > > }
> > > >
> > > > @@ -2130,7 +2178,7 @@ static int setup_tdparams(struct kvm *kvm, struct td_params *td_params,
> > > > td_params->tsc_frequency = TDX_TSC_KHZ_TO_25MHZ(kvm->arch.default_tsc_khz);
> > > >
> > > > setup_tdparams_eptp_controls(cpuid, td_params);
> > > > - setup_tdparams_cpuids(tdsysinfo, cpuid, td_params);
> > > > + setup_tdparams_cpuids(kvm, tdsysinfo, cpuid, td_params);
> > > > ret = setup_tdparams_xfam(cpuid, td_params);
> > > > if (ret)
> > > > return ret;
> > > > @@ -2332,6 +2380,11 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > > > if (cmd->flags)
> > > > return -EINVAL;
> > > >
> > > > + kvm_tdx->cpuid = kzalloc(sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
> > > > + GFP_KERNEL);
> > > > + if (!kvm_tdx->cpuid)
> > > > + return -ENOMEM;
> > > > +
> > > > init_vm = kzalloc(sizeof(*init_vm) +
> > > > sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
> > > > GFP_KERNEL);
> > >
> > > kfree(kvm_tdx->cpuid) is required in the error handling path of tdx_td_init().
> >
> >
> > No need. tdx_vm_free() frees it. Not here.
>
> From a perspective of correctness, Yes. But there seems different kinds of
> strategies of function error handling path going on in this patch series.
> Taking __tdx_td_init() as an example, tdx_vm_free() is immediately called
> in its error handling path. But, the error handling below the allocation
> of cpuid will be deferred to the late VM teardown path in this patch. I am
> confused of the strategy of common error handling path, what is the
> preferred error handling strategy? Deferring or immediate handling?
>
> Second, it is not graceful to defer the error handling to the teardown
> path all the time even they seem work: 1) Readability. It looks like a
> problematic error handling at a first glance. 2) Error handling path and
> teardown path are for different purposes. Separating the concerns brings
> clearer code organization and better maintainability. 3) Testability,
> thinking about an error injection test for tdx_td_init(). Un-freed
> kvm_tdx->cpuid will look like a memory leaking in this case and needs to
> be noted as "free is in another function". It just makes the case more
> complicated.
>
> Third, I believe a static code scanner will complain about it.
I noticed that it results in memory leak when KVM_TDX_INIT_VM after
KVM_TDX_INIT_VM error. So I come up with the following to fix it with immediate
handling.
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index d392262d8605..55cf07a72332 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -3549,16 +3549,21 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
if (cmd->flags)
return -EINVAL;
+ WARN_ON_ONCE(kvm_tdx->cpuid);
kvm_tdx->cpuid = kzalloc(sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
GFP_KERNEL);
- if (!kvm_tdx->cpuid)
- return -ENOMEM;
+ if (!kvm_tdx->cpuid) {
+ ret = -ENOMEM;
+ goto out;
+ }
init_vm = kzalloc(sizeof(*init_vm) +
sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
GFP_KERNEL);
- if (!init_vm)
- return -ENOMEM;
+ if (!init_vm) {
+ ret = -ENOMEM;
+ goto out;
+ }
if (copy_from_user(init_vm, (void __user *)cmd->data, sizeof(*init_vm))) {
ret = -EFAULT;
goto out;
@@ -3604,6 +3609,11 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
out:
/* kfree() accepts NULL. */
+ if (ret) {
+ kfree(kvm_tdx->cpuid);
+ kvm_tdx->cpuid = NULL;
+ kvm_tdx->cpuid_nent = 0;
+ }
kfree(init_vm);
kfree(td_params);
return ret;
--
Isaku Yamahata <isaku.yamahata@...il.com>
Powered by blists - more mailing lists