linux-kernel - Re: [PATCH] KVM: x86: Drop "cache" from user return MSR setter that skips WRMSR

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] [day] [month] [year] [list]

Message-ID: <aPXqfvFtT7CchfN+@yzhao56-desk.sh.intel.com>
Date: Mon, 20 Oct 2025 15:53:34 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Hou Wenlong <houwenlong.hwl@...group.com>, Paolo Bonzini
	<pbonzini@...hat.com>, <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Xiaoyao Li <xiaoyao.li@...el.com>, Rick Edgecombe
	<rick.p.edgecombe@...el.com>
Subject: Re: [PATCH] KVM: x86: Drop "cache" from user return MSR setter that
 skips WRMSR

On Thu, Oct 16, 2025 at 06:41:03AM -0700, Sean Christopherson wrote:
> On Thu, Oct 16, 2025, Hou Wenlong wrote:
> > On Thu, Oct 16, 2025 at 04:58:17PM +0800, Yan Zhao wrote:
> > > On Wed, Oct 15, 2025 at 09:19:00AM -0700, Sean Christopherson wrote:
> > > > +	/*
> > > > +	 * Leave the user-return notifiers as-is when disabling virtualization
> > > > +	 * for reboot, i.e. when disabling via IPI function call, and instead
> > > > +	 * pin kvm.ko (if it's a module) to defend against use-after-free (in
> > > > +	 * the *very* unlikely scenario module unload is racing with reboot).
> > > > +	 * On a forced reboot, tasks aren't frozen before shutdown, and so KVM
> > > > +	 * could be actively modifying user-return MSR state when the IPI to
> > > > +	 * disable virtualization arrives.  Handle the extreme edge case here
> > > > +	 * instead of trying to account for it in the normal flows.
> > > > +	 */
> > > > +	if (in_task() || WARN_ON_ONCE(!kvm_rebooting))
> > > kvm_offline_cpu() may be invoked when irq is enabled.
> > > So does it depend on [1]?
> > > 
> > > [1] https://lore.kernel.org/kvm/aMirvo9Xly5fVmbY@google.com/
> > >
> > 
> > Actually, kvm_offline_cpu() can't be interrupted by kvm_shutdown().
> > syscore_shutdown() is always called after
> > migrate_to_reboot_cpu(), which internally waits for currently running
> > CPU hotplug to complete, as described in [*].
Got it. Thanks!

> > [*] https://lore.kernel.org/kvm/dd4b8286774df98d58b5048e380b10d4de5836af.camel@intel.com
> > 
> > 
> > > > +		drop_user_return_notifiers();
> > > > +	else
> > > > +		__module_get(THIS_MODULE);
> > > Since vm_vm_fops holds ref of module kvm_intel, and drop_user_return_notifiers()
> > > is called in kvm_destroy_vm() or kvm_exit():
> > > 
> > > kvm_destroy_vm/kvm_exit
> > >   kvm_disable_virtualization
> > >     kvm_offline_cpu
> > >       kvm_disable_virtualization_cpu
> > >         drop_user_return_notifiers
> > > 
> > > also since fire_user_return_notifiers() executes with irq disabled, is it
> > > necessary to pin kvm.ko?
> 
> Pinning kvm.ko is necessary because kvm_disable_virtualization_cpu() will bail
> early due to virtualization_enabled being false (it will have been cleared by
> the IPI call from kvm_shutdown()).  We could try figuring out a way around that,
> but I don't see an easy solution, and in practice I can't think of any meaningful
> downside to pinning kvm.ko.
You are right!

> I don't want to leave virtualization_enabled set because that's completely wrong
> for everything except x86's user-return MSRs, which aren't even strictly related
> to enabling virtualization.
> 
> I considered calling drop_user_return_notifiers() directly from kvm_exit(), but
> that would require more special-case code, and it would mean blasting an IPI to
> all CPUs, which seems like a bad idea when we know the system is trying to reboot.
Agreed. Thanks for the explanation!