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

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

[Tadeusz Struk]

Syzbot reported an use-after-free bug in update_accessed_dirty_bits().
Fix this by checking if the memremap'ed pointer is still valid.

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;
 		}
-- 
2.34.1

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

[Paolo Bonzini]

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?

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;
>   		}

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

[Sean Christopherson]

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;
> >   		}
> 

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

[Tadeusz Struk]

On 1/24/22 10:29, Sean Christopherson wrote:
> 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.
> 

The issue here is not related to /dev/mem, but binder allocated memory, which is
yet another special mapping use case. In this case the condition

if (!vma || !(vma->vm_flags & VM_PFNMAP))

doesn't cover this special mappings. Adding the access_ok() was my something
that fixed the use-after-free issue for me, and since I didn't have anything
better I thought I will send an RFC to start some discussion.
After some more debugging I came up with the bellow.
Will that be more acceptable?


diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 5b5bdac97c7b..0f03e5401a98 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -167,7 +167,7 @@ static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct 
kvm_mmu *mmu,

                 mmap_read_lock(current->mm);
                 vma = find_vma_intersection(current->mm, vaddr, vaddr + PAGE_SIZE);
-               if (!vma || !(vma->vm_flags & VM_PFNMAP)) {
+               if (!vma || !(vma->vm_flags & VM_MIXEDMAP)) {
                         mmap_read_unlock(current->mm);
                         return -EFAULT;
                 }

-- 
Thanks,
Tadeusz

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

[Sean Christopherson]

On Mon, Jan 24, 2022, Tadeusz Struk wrote:
> On 1/24/22 10:29, Sean Christopherson wrote:
> > 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.
> > 
> 
> The issue here is not related to /dev/mem, but binder allocated memory, which is
> yet another special mapping use case. In this case the condition
> 
> if (!vma || !(vma->vm_flags & VM_PFNMAP))
> 
> doesn't cover this special mappings. Adding the access_ok() was my something
> that fixed the use-after-free issue for me, and since I didn't have anything
> better I thought I will send an RFC to start some discussion.
> After some more debugging I came up with the bellow.
> Will that be more acceptable?

I'm pretty sure anything that keeps the vm_pgoff "logic" is a band-aid.  But I'm 99%
sure we can simply do cmpxchg directly on the user address, we just need to get
support for that, which has happily been posted[*].  I'll give that a shot tomorrow,
I want to convert similar code in the emulator, it'd be very nice to purge all of
this crud.

[*] https://lore.kernel.org/all/20220120160822.852009966@infradead.org

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

[Sean Christopherson]

On Tue, Jan 25, 2022, Sean Christopherson wrote:
> On Mon, Jan 24, 2022, Tadeusz Struk wrote:
> > On 1/24/22 10:29, Sean Christopherson wrote:
> > > 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.
> > > 
> > 
> > The issue here is not related to /dev/mem, but binder allocated memory, which is
> > yet another special mapping use case. In this case the condition
> > 
> > if (!vma || !(vma->vm_flags & VM_PFNMAP))
> > 
> > doesn't cover this special mappings. Adding the access_ok() was my something
> > that fixed the use-after-free issue for me, and since I didn't have anything
> > better I thought I will send an RFC to start some discussion.
> > After some more debugging I came up with the bellow.
> > Will that be more acceptable?
> 
> I'm pretty sure anything that keeps the vm_pgoff "logic" is a band-aid.  But I'm 99%
> sure we can simply do cmpxchg directly on the user address, we just need to get
> support for that, which has happily been posted[*].  I'll give that a shot tomorrow,
> I want to convert similar code in the emulator, it'd be very nice to purge all of
> this crud.
> 
> [*] https://lore.kernel.org/all/20220120160822.852009966@infradead.org

Posted a series and belatedly realized my script didn't pick up Debugged-by: to Cc
you :-/  Let me know if you want me to forward any/all of the series to you.

https://lore.kernel.org/all/20220201010838.1494405-1-seanjc@google.com

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

[Tadeusz Struk]

On 1/31/22 17:11, Sean Christopherson wrote:
> On Tue, Jan 25, 2022, Sean Christopherson wrote:
>> On Mon, Jan 24, 2022, Tadeusz Struk wrote:
>>> On 1/24/22 10:29, Sean Christopherson wrote:
>>>> 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.
>>>>
>>>
>>> The issue here is not related to /dev/mem, but binder allocated memory, which is
>>> yet another special mapping use case. In this case the condition
>>>
>>> if (!vma || !(vma->vm_flags & VM_PFNMAP))
>>>
>>> doesn't cover this special mappings. Adding the access_ok() was my something
>>> that fixed the use-after-free issue for me, and since I didn't have anything
>>> better I thought I will send an RFC to start some discussion.
>>> After some more debugging I came up with the bellow.
>>> Will that be more acceptable?
>>
>> I'm pretty sure anything that keeps the vm_pgoff "logic" is a band-aid.  But I'm 99%
>> sure we can simply do cmpxchg directly on the user address, we just need to get
>> support for that, which has happily been posted[*].  I'll give that a shot tomorrow,
>> I want to convert similar code in the emulator, it'd be very nice to purge all of
>> this crud.
>>
>> [*] https://lore.kernel.org/all/20220120160822.852009966@infradead.org
> 
> Posted a series and belatedly realized my script didn't pick up Debugged-by: to Cc
> you :-/  Let me know if you want me to forward any/all of the series to you.
> 
> https://lore.kernel.org/all/20220201010838.1494405-1-seanjc@google.com

It's fine, I can follow up using the link you sent.
I will try it tomorrow and let you know.

-- 
Thanks,
Tadeusz