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

From: Tadeusz Struk <tadeusz.struk@linaro.org>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	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, 31 Jan 2022 18:29:58 -0800	[thread overview]
Message-ID: <3789ab35-6ede-34e8-b2d0-f50f4e0f1f15@linaro.org> (raw)
In-Reply-To: <YfiIxK36yD5pQgu3@google.com>

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