linux-kernel - Re: [PATCH] KVM: x86/mmu: add lockdep check before lookup_address_in

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: <YkH7KZbamhKpCidK@google.com>
Date:   Mon, 28 Mar 2022 18:15:05 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Mingwei Zhang <mizhang@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ben Gardon <bgardon@...gle.com>,
        David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH] KVM: x86/mmu: add lockdep check before
 lookup_address_in_mm()

On Mon, Mar 28, 2022, Mingwei Zhang wrote:
> With that, I start to feel this is a bug. The issue is just so rare
> that it has never triggered a problem.
>
> lookup_address_in_mm() walks the host page table as if it is a
> sequence of _static_ memory chunks. This is clearly dangerous.

Yeah, it's broken.  The proper fix is do something like what perf uses, or maybe
just genericize and reuse the code from commit 8af26be06272
("perf/core: Fix arch_perf_get_page_size()).

> But right now,  kvm_mmu_max_mapping_level() are used in other places
> as well: kvm_mmu_zap_collapsible_spte(), which does not satisfy the
> strict requirement of walking the host page table.

The host pfn size is used only as a hueristic, so false postives/negatives are
ok, the only race that needs to be avoided is dereferencing freed page table
memory.  lookup_address_in_pgd() is really broken because it doesn't even ensure
a given PxE is READ_ONCE().  I suppose one could argue the caller is broken, but
I doubt KVM is the only user that doesn't provide the necessary protections.