linux-kernel - Re: [PATCH 1/2] KVM: x86: Mark target gfn of emulated atomic instruction as dirty

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: <Zc-XF0yQp_dDUa6f@google.com>
Date: Fri, 16 Feb 2024 09:10:47 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: David Matlack <dmatlack@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Pasha Tatashin <tatashin@...gle.com>, Michael Krebs <mkrebs@...gle.com>
Subject: Re: [PATCH 1/2] KVM: x86: Mark target gfn of emulated atomic
 instruction as dirty

On Thu, Feb 15, 2024, Sean Christopherson wrote:
> On Thu, Feb 15, 2024, David Matlack wrote:
> > On Wed, Feb 14, 2024 at 5:00 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > >
> > > When emulating an atomic access on behalf of the guest, mark the target
> > > gfn dirty if the CMPXCHG by KVM is attempted and doesn't fault.  This
> > > fixes a bug where KVM effectively corrupts guest memory during live
> > > migration by writing to guest memory without informing userspace that the
> > > page is dirty.
> > >
> > > Marking the page dirty got unintentionally dropped when KVM's emulated
> > > CMPXCHG was converted to do a user access.  Before that, KVM explicitly
> > > mapped the guest page into kernel memory, and marked the page dirty during
> > > the unmap phase.
> > >
> > > Mark the page dirty even if the CMPXCHG fails, as the old data is written
> > > back on failure, i.e. the page is still written.  The value written is
> > > guaranteed to be the same because the operation is atomic, but KVM's ABI
> > > is that all writes are dirty logged regardless of the value written.  And
> > > more importantly, that's what KVM did before the buggy commit.
> > >
> > > Huge kudos to the folks on the Cc list (and many others), who did all the
> > > actual work of triaging and debugging.
> > >
> > > Fixes: 1c2361f667f3 ("KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses")
> > 
> > I'm only half serious but... Should we just revert this commit?
> 
> No.

David, any objection to this patch?  I'd like to get this on its way to Paolo
asap, but also want to make sure we all agree this is the right solution before
doing so.