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@xxxxxxxxxx > > Thanks, > > Paolo > > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > Cc: Sean Christopherson <seanjc@xxxxxxxxxx> > > Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> > > Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx> > > Cc: Jim Mattson <jmattson@xxxxxxxxxx> > > Cc: Joerg Roedel <joro@xxxxxxxxxx> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > > Cc: Ingo Molnar <mingo@xxxxxxxxxx> > > Cc: Borislav Petkov <bp@xxxxxxxxx> > > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> > > Cc: x86@xxxxxxxxxx > > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx> > > Cc: kvm@xxxxxxxxxxxxxxx > > Cc: <stable@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Tadeusz Struk <tadeusz.struk@xxxxxxxxxx> > > --- > > 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; > > } >
