linux-kernel - Re: [PATCH v11 0/8] KVM: allow mapping non-refcounted pages

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]

Date: Thu, 14 Mar 2024 07:45:42 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: "Christian König" <christian.koenig@....com>
Cc: David Stevens <stevensd@...omium.org>, Christoph Hellwig <hch@...radead.org>, 
	Paolo Bonzini <pbonzini@...hat.com>, Yu Zhang <yu.c.zhang@...ux.intel.com>, 
	Isaku Yamahata <isaku.yamahata@...il.com>, Zhi Wang <zhi.wang.linux@...il.com>, 
	Maxim Levitsky <mlevitsk@...hat.com>, kvmarm@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org
Subject: Re: [PATCH v11 0/8] KVM: allow mapping non-refcounted pages

On Thu, Mar 14, 2024, Christian König wrote:
> Am 14.03.24 um 12:31 schrieb David Stevens:
> > On Thu, Mar 14, 2024 at 6:20 PM Christian König <christiankoenig@....com> wrote:
> > > > > > > > Well as far as I can see Christoph rejects the complexity coming with the
> > > > > > > > approach of sometimes grabbing the reference and sometimes not.
> > > > > > > Unless I've wildly misread multiple threads, that is not Christoph's objection.
> > > > > > >    From v9 (https://lore.kernel.org/all/ZRpiXsm7X6BFAU%2Fy@infradead.org):
> > > > > > > 
> > > > > > >      On Sun, Oct 1, 2023 at 11:25 PM Christoph Hellwig<hch@...radead.org>   wrote:
> > > > > > >      >
> > > > > > >      > On Fri, Sep 29, 2023 at 09:06:34AM -0700, Sean Christopherson wrote:
> > > > > > >      > > KVM needs to be aware of non-refcounted struct page memory no matter what; see
> > > > > > >      > > CVE-2021-22543 and, commit f8be156be163 ("KVM: do not allow mapping valid but
> > > > > > >      > > non-reference-counted pages").  I don't think it makes any sense whatsoever to
> > > > > > >      > > remove that code and assume every driver in existence will do the right thing.
> > > > > > >      >
> > > > > > >      > Agreed.
> > > > > > >      >
> > > > > > >      > >
> > > > > > >      > > With the cleanups done, playing nice with non-refcounted paged instead of outright
> > > > > > >      > > rejecting them is a wash in terms of lines of code, complexity, and ongoing
> > > > > > >      > > maintenance cost.
> > > > > > >      >
> > > > > > >      > I tend to strongly disagree with that, though.  We can't just let these
> > > > > > >      > non-refcounted pages spread everywhere and instead need to fix their
> > > > > > >      > usage.
> > > > > > And I can only repeat myself that I completely agree with Christoph here.
> > > > > I am so confused.  If you agree with Christoph, why not fix the TTM allocations?
> > > > Because the TTM allocation isn't broken in any way.
> > > > 
> > > > See in some configurations TTM even uses the DMA API for those
> > > > allocations and that is actually something Christoph coded.
> > > > 
> > > > What Christoph is really pointing out is that absolutely nobody should
> > > > put non-refcounted pages into a VMA, but again this isn't something
> > > > TTM does. What TTM does instead is to work with the PFN and puts that
> > > > into a VMA.
> > > > 
> > > > It's just that then KVM comes along and converts the PFN back into a
> > > > struct page again and that is essentially what causes all the
> > > > problems, including CVE-2021-22543.
> > Does Christoph's objection come from my poorly worded cover letter and
> > commit messages, then?
> 
> Yes, that could certainly be.
> 
> > Fundamentally, what this series is doing is
> > allowing pfns returned by follow_pte to be mapped into KVM's shadow
> > MMU without inadvertently translating them into struct pages.
> 
> As far as I can tell that is really the right thing to do. Yes.

Christoph,

Can you please confirm that you don't object to KVM using follow_pte() to get
PFNs which happen to have an associated struct page?  We've gone in enough circles...