linux-kernel - RE: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module init/exit functions

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, 14 Mar 2023 01:57:57 +0000
From:   "Wang, Wei W" <wei.w.wang@...el.com>
To:     Isaku Yamahata <isaku.yamahata@...il.com>
CC:     "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        David Matlack <dmatlack@...gle.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        Zhi Wang <zhi.wang.linux@...il.com>
Subject: RE: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module
 init/exit functions

On Tuesday, March 14, 2023 2:40 AM, Isaku Yamahata wrote:
> > I had a patch to fix a bug here, maybe you can take it:
> >
> > kvm_x86_vendor_init copies vt_x86_ops to kvm_x86_ops.
> > vt_x86_ops.vm_size needs to be updated before calling
> > kvm_x86_vendor_init so that kvm_x86_ops can get the correct vm_size.
> 
> Thanks for catching it.  With your patch, vm_size is always max(sizeof struct
> kvm_vmx, sizeof strut kvm_tdx) even when the admin sets kvm_intel.tdx=true
> and tdx is disabled by error.

This seems to be another bug, where enable_tdx should be set to false if tdx
fails to be enabled:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index d6a9f705a2a1..ef1e794efb97 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -4552,6 +4552,7 @@ static int tdx_module_setup(void)

        ret = tdx_enable();
        if (ret) {
+               enable_tdx = false;
                pr_info("Failed to initialize TDX module.\n");
                return ret;
        }

> 
> option 1: Ignore such waste. Your patch. The difference is small and it's only
>           the error case. Locally I have the following values.
>           sizeof(struct kvm_vmx) = 44576
>           sizeof(struct vcpu_vmx) = 10432
>           sizeof(struct kvm_tdx)= 44632
>           sizeof(struct vcpu_tdx) = 8192
>           I suspect the actual allocation size for struct kvm is same.  That's
>           the reason why I didn't hit problem.

No, the actual allocation size isn't same.
You didn’t get error notices because the kvm_tdx fields are still located
in the same page as that allocated for kvm_vmx, though that part of memory
isn’t explicitly allocated. If kvm_tdx size grows and crosses the page boundary,
you will see unexpected faults. Or if this part of memory (illegally used by
kvm_tdx) later happens to be given to other kernel components to use, you
would see random errors somewhere.

> 
> option 2: Explicitly update vm_size after kvm_x86_vendor_init()
>           struct kvm_x86_ops isn't exported.  It would be ugly.
> 
> option 3: Allow setup_hardware() to update vm_size.
>           setup_hardware(void) => setup_hardware(unsigned int *vm_size)
>           It's confusing because kvm_x86_ops.vm_size is already initialized.
> 
> Let's go with option 1(your patch).

Sounds good.