linux-kernel - Re: [PATCH v2 1/4] KVM: x86/mmu: Introduce a quirk to control memslot zap behavior

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: <ZvG1Wki4GvIyVWqB@google.com>
Date: Mon, 23 Sep 2024 11:37:14 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Yan Zhao <yan.y.zhao@...el.com>
Cc: pbonzini@...hat.com, rick.p.edgecombe@...el.com, kai.huang@...el.com, 
	isaku.yamahata@...el.com, dmatlack@...gle.com, sagis@...gle.com, 
	erdemaktas@...gle.com, graf@...zon.com, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org
Subject: Re: [PATCH v2 1/4] KVM: x86/mmu: Introduce a quirk to control memslot
 zap behavior

On Wed, Jul 03, 2024, Yan Zhao wrote:
> Introduce the quirk KVM_X86_QUIRK_SLOT_ZAP_ALL to allow users to select
> KVM's behavior when a memslot is moved or deleted for KVM_X86_DEFAULT_VM
> VMs. Make sure KVM behave as if the quirk is always disabled for
> non-KVM_X86_DEFAULT_VM VMs.
 
...

> Suggested-by: Kai Huang <kai.huang@...el.com>
> Suggested-by: Sean Christopherson <seanjc@...gle.com>

Bad Sean, bad.

> +/*
> + * Zapping leaf SPTEs with memslot range when a memslot is moved/deleted.
> + *
> + * Zapping non-leaf SPTEs, a.k.a. not-last SPTEs, isn't required, worst
> + * case scenario we'll have unused shadow pages lying around until they
> + * are recycled due to age or when the VM is destroyed.
> + */
> +static void kvm_mmu_zap_memslot_leafs(struct kvm *kvm, struct kvm_memory_slot *slot)
> +{
> +	struct kvm_gfn_range range = {
> +		.slot = slot,
> +		.start = slot->base_gfn,
> +		.end = slot->base_gfn + slot->npages,
> +		.may_block = true,
> +	};
> +	bool flush = false;
> +
> +	write_lock(&kvm->mmu_lock);
> +
> +	if (kvm_memslots_have_rmaps(kvm))
> +		flush = kvm_handle_gfn_range(kvm, &range, kvm_zap_rmap);

This, and Paolo's merged variant, break shadow paging.  As was tried in commit
4e103134b862 ("KVM: x86/mmu: Zap only the relevant pages when removing a memslot"),
all shadow pages, i.e. non-leaf SPTEs, need to be zapped.  All of the accounting
for a shadow page is tied to the memslot, i.e. the shadow page holds a reference
to the memslot, for all intents and purposes.  Deleting the memslot without removing
all relevant shadow pages results in NULL pointer derefs when tearing down the VM.

Note, that commit is/was buggy, and I suspect my follow-up attempt[*] was as well.
https://lore.kernel.org/all/20190820200318.GA15808@linux.intel.com

Rather than trying to get this functional for shadow paging (which includes nested
TDP), I think we should scrap the quirk idea and simply make this the behavior for
S-EPT and nothing else.

 BUG: kernel NULL pointer dereference, address: 00000000000000b0
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 6085f43067 P4D 608c080067 PUD 608c081067 PMD 0 
 Oops: Oops: 0000 [#1] SMP NOPTI
 CPU: 79 UID: 0 PID: 187063 Comm: set_memory_regi Tainted: G        W          6.11.0-smp--24867312d167-cpl #395
 Tainted: [W]=WARN
 Hardware name: Google Astoria/astoria, BIOS 0.20240617.0-0 06/17/2024
 RIP: 0010:__kvm_mmu_prepare_zap_page+0x3a9/0x7b0 [kvm]
 Code:  <48> 8b 8e b0 00 00 00 48 8b 96 e0 00 00 00 48 c1 e9 09 48 29 c8 8b
 RSP: 0018:ff314a25b19f7c28 EFLAGS: 00010212
 Call Trace:
  <TASK>
  kvm_arch_flush_shadow_all+0x7a/0xf0 [kvm]
  kvm_mmu_notifier_release+0x6c/0xb0 [kvm]
  mmu_notifier_unregister+0x85/0x140
  kvm_put_kvm+0x263/0x410 [kvm]
  kvm_vm_release+0x21/0x30 [kvm]
  __fput+0x8d/0x2c0
  __se_sys_close+0x71/0xc0
  do_syscall_64+0x83/0x160
  entry_SYSCALL_64_after_hwframe+0x76/0x7e