On Wed, Feb 09, 2022, Paolo Bonzini wrote: > If kvm_mmu_free_roots encounters a PAE page table where a 64-bit page > table is expected, the result is a NULL pointer dereference. Instead > just WARN and exit. This confused the heck out of me, because we obviously free PAE page tables. What we don't do is back the root that gets shoved into CR3 with a shadow page. It'd be especially confusing without the context that this WARN was helpful during related development, as it's not super obvious why mmu_free_root_page() is a special snowflake and deserves a WARN. Something like this? WARN and bail if KVM attempts to free a root that isn't backed by a shadow page. KVM allocates a bare page for "special" roots, e.g. when using PAE paging or shadowing 2/3/4-level page tables with 4/5-level, and so root_hpa will be valid but won't be backed by a shadow page. It's all too easy to blindly call mmu_free_root_page() on root_hpa, be nice and WARN instead of crashing KVM and possibly the kernel. > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> > --- > arch/x86/kvm/mmu/mmu.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 7b5765ced928..d0f2077bd798 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -3201,6 +3201,8 @@ static void mmu_free_root_page(struct kvm *kvm, hpa_t *root_hpa, > return; > > sp = to_shadow_page(*root_hpa & PT64_BASE_ADDR_MASK); > + if (WARN_ON(!sp)) Should this be KVM_BUG_ON()? I.e. when you triggered these, would continuing on potentially corrupt guest data, or was it truly benign-ish? > + return; > > if (is_tdp_mmu_page(sp)) > kvm_tdp_mmu_put_root(kvm, sp, false); > -- > 2.31.1 > >
