linux-kernel - Re: [PATCH v3 2/3] KVM: x86: Use kvzalloc() to allocate VM struct

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 for Android: free password hash cracker in your pocket

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

Message-ID: <572c0f5ce627384b6441b64b9fa036b202d430b7.camel@intel.com>
Date: Fri, 23 May 2025 11:31:10 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "seanjc@...gle.com" <seanjc@...gle.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "vipinsh@...gle.com" <vipinsh@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] KVM: x86: Use kvzalloc() to allocate VM struct

On Thu, 2025-05-22 at 06:40 -0700, Sean Christopherson wrote:
> On Wed, May 21, 2025, Kai Huang wrote:
> > On Wed, 2025-05-21 at 10:12 -0700, Sean Christopherson wrote:
> > > > e.g., if we export kvm_x86_ops, we could unwind them.
> > > 
> > > Maaaybe.  I mean, yes, we could fully unwind kvm_x86_ops, but doing so would make
> > > the overall code far more brittle.  E.g. simply updating kvm_x86_ops won't suffice,
> > > as the static_calls also need to be patched, and we would have to be very careful
> > > not to touch anything in kvm_x86_ops that might have been consumed between here
> > > and the call to tdx_bringup().
> > 
> > Right.  Maybe exporting kvm_ops_update() is better.
> 
> A bit, but KVM would still need to be careful not to modify the parts of
> vt_x86_ops that have already been consumed.
> 
> While I agree that leaving TDX breadcrumbs in kvm_x86_ops when TDX is disabled is
> undesirable, the behavior is known, i.e. we know exactly what TDX state is being
> left behind.  And failure to load the TDX Module should be very, very rare for
> any setup that is actually trying to enable TDX.

This is true.  Agreed.

> 
> Whereas providing a way to modify kvm_x86_ops creates the possibility for "bad"
> updates.  KVM's initialization code is a lot like the kernel's boot code (and
> probably most bootstrapping code): it's inherently fragile because avoiding
> dependencies is practically impossible.
> 
> E.g. I ran into a relevant ordering problem[*] just a few days ago, where checking
> for VMX capabilities during PMU initialization always failed because the VMCS
> config hadn't yet been parsed.  Those types of bugs are especially dangerous
> because they're very easy to overlook when modifying existing code, e.g. the
> only sign that anything is broken is an optional feature being missing.
> 
> [*] https://lore.kernel.org/all/aCU2YEpU0dOk7RTk@google.com

Right.  No argument around this.  I agree if there are multiple features wanting
to update then it could be dangerous.  Thanks for the insight :-)