Re: [PATCH v14 111/113] RFC: KVM: x86, TDX: Add check for setting CPUID

[Isaku Yamahata <isaku.yamahata@gmail.com>]

On Mon, Jun 05, 2023 at 10:25:11AM +0800,
Zhi Wang <zhi.wang.linux@gmail.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@gmail.com>