linux-kernel - Re: [PATCH v3 12/21] KVM: X86: Implement ring-based dirty memory tracking

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: <20200120072915.GD380565@xz-x1>
Date:   Mon, 20 Jan 2020 15:29:15 +0800
From:   Peter Xu <peterx@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Christophe de Dinechin <dinechin@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Yan Zhao <yan.y.zhao@...el.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Kevin Kevin <kevin.tian@...el.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        Lei Cao <lei.cao@...atus.com>
Subject: Re: [PATCH v3 12/21] KVM: X86: Implement ring-based dirty memory
 tracking

On Sun, Jan 19, 2020 at 05:12:35AM -0500, Michael S. Tsirkin wrote:
> On Sun, Jan 19, 2020 at 10:09:53AM +0100, Paolo Bonzini wrote:
> > On 09/01/20 20:15, Peter Xu wrote:
> > > Regarding dropping the indices: I feel like it can be done, though we
> > > probably need two extra bits for each GFN entry, for example:
> > > 
> > >   - Bit 0 of the GFN address to show whether this is a valid publish
> > >     of dirty gfn
> > > 
> > >   - Bit 1 of the GFN address to show whether this is collected by the
> > >     user
> > 
> > We can use bit 62 and 63 of the GFN.
> 
> If we are short on bits we can just use 1 bit. E.g. set if
> userspace has collected the GFN.

I'm still unsure whether we can use only one bit for this.  Say,
otherwise how does the userspace knows the entry is valid?  For
example, the entry with all zeros ({.slot = 0, gfn = 0}) could be
recognized as a valid dirty page on slot 0 gfn 0, even if it's
actually an unused entry.

> 
> > I think this can be done in a secure way.  Later in the thread you say:
> > 
> > > We simply check fetch_index (sorry I
> > > meant this when I said reset_index, anyway it's the only index that we
> > > expose to userspace) to make sure:
> > > 
> > >   reset_index <= fetch_index <= dirty_index
> > 
> > So this means that KVM_RESET_DIRTY_RINGS should only test the "collected
> > by user" flag on dirty ring entries between reset_index and dirty_index.
> > 
> > Also I would make it
> > 
> >    00b (invalid GFN) ->
> >      01b (valid gfn published by kernel, which is dirty) ->
> >        1*b (gfn dirty page collected by userspace) ->
> >          00b (gfn reset by kernel, so goes back to invalid gfn)
> > That is 10b and 11b are equivalent.  The kernel doesn't read that bit if
> > userspace has collected the page.

Yes "1*b" is good too (IMHO as long as we can define three states for
an entry).  However do you want me to change to that?  Note that I
still think we need to read the rest of the field (in this case,
"slot" and "gfn") besides the two bits to do re-protect.  Should we
trust that unconditionally if writable?

Thanks,

-- 
Peter Xu