Re: There is a null-ptr-deref bug in kvm_dirty_ring_get in virt/kvm/dirty_ring.c

From: "Woodhouse, David" <dwmw@amazon.co.uk>
To: "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"butterflyhuangxx@gmail.com" <butterflyhuangxx@gmail.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: There is a null-ptr-deref bug in kvm_dirty_ring_get in virt/kvm/dirty_ring.c
Date: Wed, 17 Nov 2021 18:10:11 +0000	[thread overview]
Message-ID: <f83554ba7bfea1ab45d316db4b68569382727175.camel@amazon.co.uk> (raw)
In-Reply-To: <20eddd70-7abb-e1a8-a003-62ed08fc1cac@redhat.com>

On Wed, 2021-11-17 at 17:49 +0100, Paolo Bonzini wrote:
> On 11/17/21 10:46, Woodhouse, David wrote:
> > > The remaining
> > > option would be just "do not mark the page as dirty if the ring buffer
> > > is active".  This is feasible because userspace itself has passed the
> > > shared info gfn; but again, it's ugly...
> > I think I am coming to quite like that 'remaining option' as long as we
> > rephrase it as follows:
> > 
> >   KVM does not mark the shared_info page as dirty, and userspace is
> >   expected to*assume*  that it is dirty at all times. It's used for
> >   delivering event channel interrupts and the overhead of marking it
> >   dirty each time is just pointless.
> 
> For the case of dirty-bitmap, one solution could be to only set a bool
> and actually mark the page dirty lazily, at the time of
> KVM_GET_DIRTY_LOG.  For dirty-ring, I agree that it's easiest if
> userspace just "knows" the page is dirty.

TBH we get that former behaviour for free if we just do the access via
the shiny new gfn_to_pfn_cache. The page is marked dirty once, when the
cache is invalidated. I was actually tempted to avoid even setting the
dirty bit even when we write to the shinfo page.

None of which *immediately* solves it for the wall clock part because
we just call the same kvm_write_wall_clock() that the KVM MSR version
invokes, and that uses kvm_write_guest().

I think I'll just reimplement the interesting part of
kvm_write_wall_clock() directly in kvm_xen_shared_info_init(). I don't
much like the duplication but it isn't much and it's the simplest
option I see. And it actually simplifies kvm_write_wall_clock() which
no longer needs the 'sec_hi_ofs' argument. And the Xen version is also
simpler because it can just access the kernel mapping directly.

Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.