LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	syzbot+200c08e88ae818f849ce@syzkaller.appspotmail.com
Subject: Re: [PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()"
Date: Wed, 1 Sep 2021 16:09:09 +0000	[thread overview]
Message-ID: <YS+lpeqHHgoA6W8A@google.com> (raw)
In-Reply-To: <87pmtsog4c.fsf@vitty.brq.redhat.com>

On Wed, Sep 01, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > Revert a misguided illegal GPA check when "translating" a non-nested GPA.
> > The check is woefully incomplete as it does not fill in @exception as
> > expected by all callers, which leads to KVM attempting to inject a bogus
> > exception, potentially exposing kernel stack information in the process.
> >
> >  WARNING: CPU: 0 PID: 8469 at arch/x86/kvm/x86.c:525 exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
> >  CPU: 1 PID: 8469 Comm: syz-executor531 Not tainted 5.14.0-rc7-syzkaller #0
> >  RIP: 0010:exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
> >  Call Trace:
> >   x86_emulate_instruction+0xef6/0x1460 arch/x86/kvm/x86.c:7853
> >   kvm_mmu_page_fault+0x2f0/0x1810 arch/x86/kvm/mmu/mmu.c:5199
> >   handle_ept_misconfig+0xdf/0x3e0 arch/x86/kvm/vmx/vmx.c:5336
> >   __vmx_handle_exit arch/x86/kvm/vmx/vmx.c:6021 [inline]
> >   vmx_handle_exit+0x336/0x1800 arch/x86/kvm/vmx/vmx.c:6038
> >   vcpu_enter_guest+0x2a1c/0x4430 arch/x86/kvm/x86.c:9712
> >   vcpu_run arch/x86/kvm/x86.c:9779 [inline]
> >   kvm_arch_vcpu_ioctl_run+0x47d/0x1b20 arch/x86/kvm/x86.c:10010
> >   kvm_vcpu_ioctl+0x49e/0xe50 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3652
> >
> > The bug has escaped notice because practically speaking the GPA check is
> > useless.  The GPA check in question only comes into play when KVM is
> > walking guest page tables (or "translating" CR3), and KVM already handles
> > illegal GPA checks by setting reserved bits in rsvd_bits_mask for each
> > PxE, or in the case of CR3 for loading PTDPTRs, manually checks for an
> > illegal CR3.  This particular failure doesn't hit the existing reserved
> > bits checks because syzbot sets guest.MAXPHYADDR=1, and IA32 architecture
> > simply doesn't allow for such an absurd MAXPHADDR, e.g. 32-bit paging
> 
> "MAXPHYADDR"

Gah, I'm pretty sure I mistype that 50% of the time.

> I'm, however, wondering if it would also make sense to forbid setting
> nonsensical MAXPHYADDR, something like (compile-only tested):

That crossed my mind as well, but I actually think letting userspace provide
garbage is a good thing in this case.  From a kernel-safety perspective, KVM
does not and _should_ not make any assumptions about guest.MAXPHYADDR.  IMO, the
added test coverage gaind by allowing truly outrageous values outweighs the
potential danger, e.g. this bogus code would likely have gone unnoticed forever.

  reply	other threads:[~2021-09-01 16:09 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 16:42 [PATCH 0/3] KVM: x86: guest.MAXPHYADDR fix and related cleanup Sean Christopherson
2021-08-31 16:42 ` [PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()" Sean Christopherson
2021-09-01  8:27   ` Vitaly Kuznetsov
2021-09-01 16:09     ` Sean Christopherson [this message]
2021-09-06 10:18   ` Paolo Bonzini
2021-08-31 16:42 ` [PATCH 2/3] KVM: x86: Subsume nested GPA read helper into load_pdptrs() Sean Christopherson
2021-08-31 16:42 ` [PATCH 3/3] KVM: x86: Simplify retrieving the page offset when loading PDTPRs Sean Christopherson
2021-09-23 16:22 ` [PATCH 0/3] KVM: x86: guest.MAXPHYADDR fix and related cleanup Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YS+lpeqHHgoA6W8A@google.com \
    --to=seanjc@google.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=syzbot+200c08e88ae818f849ce@syzkaller.appspotmail.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --subject='Re: [PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()"' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).