linux-kernel - Re: [PATCH v2 10/49] KVM: x86: Drop now-redundant MAXPHYADDR and GPA rsvd bits from vCPU creation

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]

Message-ID: <ZoxDqKdh37qpm-HQ@google.com>
Date: Mon, 8 Jul 2024 12:53:12 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Hou Wenlong <houwenlong.hwl@...group.com>, 
	Kechen Lu <kechenl@...dia.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	Binbin Wu <binbin.wu@...ux.intel.com>, Yang Weijiang <weijiang.yang@...el.com>, 
	Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 10/49] KVM: x86: Drop now-redundant MAXPHYADDR and GPA
 rsvd bits from vCPU creation

On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > Drop the manual initialization of maxphyaddr and reserved_gpa_bits during
> > vCPU creation now that kvm_arch_vcpu_create() unconditionally invokes
> > kvm_vcpu_after_set_cpuid(), which handles all such CPUID caching.
> > 
> > None of the helpers between the existing code in kvm_arch_vcpu_create()
> > and the call to kvm_vcpu_after_set_cpuid() consume maxphyaddr or
> > reserved_gpa_bits (though auditing vmx_vcpu_create() and svm_vcpu_create()
> > isn't exactly easy).  And even if that weren't the case, KVM _must_
> > refresh any affected state during kvm_vcpu_after_set_cpuid(), e.g. to
> > correctly handle KVM_SET_CPUID2.  In other words, this can't introduce a
> > new bug, only expose an existing bug (of which there don't appear to be
> > any).
> 
> 
> IMHO the change is not as bulletproof as claimed:
> 
> If some code does access the uninitialized state (e.g vcpu->arch.maxphyaddr
> which will be zero, I assume), in between these calls, then even though later
> the correct CPUID will be set and should override the incorrect state set
> earlier, the problem *is* that the mentioned code will have to deal with non
> architecturally possible value (e.g maxphyaddr == 0) which might cause a bug
> in it.
>
> Of course such code currently doesn't exist, so it works but it can fail in
> the future.

Similar to not consuming a null cpuid_entries, any such future bug should never
escape developer testing since this is a very fixed sequence.  And practically
speaking, completely closing these holes isn't feasible because it's impossible
to initialize everything simultaneously, i.e. some amount of code will always
need to execute with zero-initialized vCPU state.

> How about we move the call to kvm_vcpu_after_set_cpuid upward?

A drop-in replacement was my preference too, but it doesn't work.  :-/
kvm_vcpu_after_set_cpuid() needs to be called after vcpu_load(), e.g. VMX's
hook will do VMWRITE.