[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 5 May 2023 09:28:08 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: "Mickaël Salaün" <mic@...ikod.net>
Cc: Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H . Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
Kees Cook <keescook@...omium.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Alexander Graf <graf@...zon.com>,
Forrest Yuan Yu <yuanyu@...gle.com>,
James Morris <jamorris@...ux.microsoft.com>,
John Andersen <john.s.andersen@...el.com>,
Liran Alon <liran.alon@...cle.com>,
"Madhavan T . Venkataraman" <madvenka@...ux.microsoft.com>,
Marian Rotariu <marian.c.rotariu@...il.com>,
"Mihai Donțu" <mdontu@...defender.com>,
"Nicușor Cîțu" <nicu.citu@...oud.com>,
Rick Edgecombe <rick.p.edgecombe@...el.com>,
Thara Gopinath <tgopinath@...rosoft.com>,
Will Deacon <will@...nel.org>,
Zahra Tarkhani <ztarkhani@...rosoft.com>,
"Ștefan Șicleru" <ssicleru@...defender.com>,
dev@...ts.cloudhypervisor.org, kvm@...r.kernel.org,
linux-hardening@...r.kernel.org, linux-hyperv@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, qemu-devel@...gnu.org,
virtualization@...ts.linux-foundation.org, x86@...nel.org,
xen-devel@...ts.xenproject.org
Subject: Re: [PATCH v1 2/9] KVM: x86/mmu: Add support for prewrite page tracking
On Fri, May 05, 2023, Micka�l Sala�n wrote:
> diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h
> index eb186bc57f6a..a7fb4ff888e6 100644
> --- a/arch/x86/include/asm/kvm_page_track.h
> +++ b/arch/x86/include/asm/kvm_page_track.h
> @@ -3,6 +3,7 @@
> #define _ASM_X86_KVM_PAGE_TRACK_H
>
> enum kvm_page_track_mode {
> + KVM_PAGE_TRACK_PREWRITE,
Heh, just when I decide to finally kill off support for multiple modes[1] :-)
My assessment from that changelog still holds true for this case:
Drop "support" for multiple page-track modes, as there is no evidence
that array-based and refcounted metadata is the optimal solution for
other modes, nor is there any evidence that other use cases, e.g. for
access-tracking, will be a good fit for the page-track machinery in
general.
E.g. one potential use case of access-tracking would be to prevent guest
access to poisoned memory (from the guest's perspective). In that case,
the number of poisoned pages is likely to be a very small percentage of
the guest memory, and there is no need to reference count the number of
access-tracking users, i.e. expanding gfn_track[] for a new mode would be
grossly inefficient. And for poisoned memory, host userspace would also
likely want to trap accesses, e.g. to inject #MC into the guest, and that
isn't currently supported by the page-track framework.
A better alternative for that poisoned page use case is likely a
variation of the proposed per-gfn attributes overlay (linked), which
would allow efficiently tracking the sparse set of poisoned pages, and by
default would exit to userspace on access.
Of particular relevance:
- Using the page-track machinery is inefficient because the guest is likely
going to write-protect a minority of its memory. And this
select KVM_EXTERNAL_WRITE_TRACKING if KVM
is particularly nasty because simply enabling HEKI in the Kconfig will cause
KVM to allocate rmaps and gfn tracking.
- There's no need to reference count the protection, i.e. 15 of the 16 bits of
gfn_track are dead weight.
- As proposed, adding a second "mode" would double the cost of gfn tracking.
- Tying the protections to the memslots will create an impossible-to-maintain
ABI because the protections will be lost if the owning memslot is deleted and
recreated.
- The page-track framework provides incomplete protection and will lead to an
ongoing game of whack-a-mole, e.g. this patch catches the obvious cases by
adding calls to kvm_page_track_prewrite(), but misses things like kvm_vcpu_map().
- The scaling and maintenance issues will only get worse if/when someone tries
to support dropping read and/or execute permissions, e.g. for execute-only.
- The code is x86-only, and is likely to stay that way for the foreseeable
future.
The proposed alternative is to piggyback the memory attributes implementation[2]
that is being added (if all goes according to plan) for confidential VMs. This
use case (dropping permissions) came up not too long ago[3], which is why I have
a ready-made answer).
I have no doubt that we'll need to solve performance and scaling issues with the
memory attributes implementation, e.g. to utilize xarray multi-range support
instead of storing information on a per-4KiB-page basis, but AFAICT, the core
idea is sound. And a very big positive from a maintenance perspective is that
any optimizations, fixes, etc. for one use case (CoCo vs. hardening) should also
benefit the other use case.
[1] https://lore.kernel.org/all/20230311002258.852397-22-seanjc@google.com
[2] https://lore.kernel.org/all/Y2WB48kD0J4VGynX@google.com
[3] https://lore.kernel.org/all/Y1a1i9vbJ%2FpVmV9r@google.com
Powered by blists - more mailing lists