Am 14.03.24 um 12:31 schrieb David Stevens:
On Thu, Mar 14, 2024 at 6:20 PM Christian König
<christian.koenig@xxxxxxx> wrote:
Sending that out once more since the AMD email servers have converted it
to HTML mail once more :(
Grrr,
Christian.
Am 14.03.24 um 10:18 schrieb Christian König:
Am 13.03.24 um 18:26 schrieb Sean Christopherson:
On Wed, Mar 13, 2024, Christian König wrote:
Am 13.03.24 um 16:47 schrieb Sean Christopherson:
[SNIP]
It can trivially be that userspace only maps 4KiB of some 2MiB piece of
memory the driver has allocate.
I.e. Christoph is (implicitly) saying that instead of modifying KVM to play nice,
we should instead fix the TTM allocations. And David pointed out that that was
tried and got NAK'd.
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@xxxxxxxxxxxxx):
On Sun, Oct 1, 2023 at 11:25 PM Christoph Hellwig<hch@xxxxxxxxxxxxx> 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.
If I'm understand this discussion correctly, since KVM's shadow MMU is hooked
up to MMU notifiers, this shouldn't be controversial. However, my
cover letter got a little confused because KVM is currently doing
something that it sounds like it shouldn't - translating pfns returned
by follow_pte into struct pages with kvm_try_get_pfn. Because of that,
the specific type of pfns that don't work right now are pfn_valid() &&
!PG_Reserved && !page_ref_count() - what I called the non-refcounted
pages in a bad choice of words. If that's correct, then perhaps this
series should go a little bit further in modifying
hva_to_pfn_remapped, but it isn't fundamentally wrong.
Completely agree. In my thinking when you go a step further and offload
grabbing the page reference to get_user_pages() then you are always on
the save side.
Because then it is no longer the responsibility of the KVM code to get
all the rules around that right, instead you are relying on a core
functionality which should (at least in theory) do the correct thing.
Regards,
Christian.
-David
