Re: [PATCH RESEND] KVM: x86/mmu: fix UAF in paging_update_accessed_dirty_bits

From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Tadeusz Struk <tadeusz.struk@linaro.org>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	kvm@vger.kernel.org, stable@vger.kernel.org,
	syzbot+6cde2282daa792c49ab8@syzkaller.appspotmail.com
Subject: Re: [PATCH RESEND] KVM: x86/mmu: fix UAF in paging_update_accessed_dirty_bits
Date: Mon, 24 Jan 2022 18:29:29 +0000	[thread overview]
Message-ID: <Ye7wCbRpcbU2G4qH@google.com> (raw)
In-Reply-To: <6fd96538-b767-41e8-0cca-5b9be1dbb1c9@redhat.com>

On Mon, Jan 24, 2022, Paolo Bonzini wrote:
> On 1/24/22 18:26, Tadeusz Struk wrote:
> > Syzbot reported an use-after-free bug in update_accessed_dirty_bits().
> > Fix this by checking if the memremap'ed pointer is still valid.
> 
> access_ok only checks that the pointer is in the userspace range.  Is this
> correct?  And if so, what are the exact circumstances in which access_ok
> returns a non-NULL but also non-userspace address?

I "objected" to this patch in its initial posting[*].  AFAICT adding access_ok()
is just masking a more egregious bug where interpretation of vm_pgoff as a PFN
base is flat out wrong except for select backing stores that use VM_PFNMAP.  In
other words, the vm_pgoff hack works for the /dev/mem use case, but it is wrong
in general.

[*] https://lore.kernel.org/all/Ybp0naX%2FZTG9FNEa@google.com
> 
> Thanks,
> 
> Paolo
> 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Cc: Wanpeng Li <wanpengli@tencent.com>
> > Cc: Jim Mattson <jmattson@google.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: x86@kernel.org
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: kvm@vger.kernel.org
> > Cc: <stable@vger.kernel.org>
> > Fixes: bd53cb35a3e9 ("X86/KVM: Handle PFNs outside of kernel reach when touching GPTEs")
> > Link: https://syzkaller.appspot.com/bug?id=6cb6102a0a7b0c52060753dd62d070a1d1e71347
> > Reported-by: syzbot+6cde2282daa792c49ab8@syzkaller.appspotmail.com
> > Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org>
> > ---
> >   arch/x86/kvm/mmu/paging_tmpl.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> > index 5b5bdac97c7b..d25b72d7b1b1 100644
> > --- a/arch/x86/kvm/mmu/paging_tmpl.h
> > +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> > @@ -174,7 +174,7 @@ static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> >   		pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> >   		paddr = pfn << PAGE_SHIFT;
> >   		table = memremap(paddr, PAGE_SIZE, MEMREMAP_WB);
> > -		if (!table) {
> > +		if (!table || !access_ok(table, PAGE_SIZE)) {
> >   			mmap_read_unlock(current->mm);
> >   			return -EFAULT;
> >   		}
> 