Message ID | 20240313012913.2395414-1-zhangpeng362@huawei.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | filemap: replace pte_offset_map() with pte_offset_map_nolock() | expand |
On Wed, 13 Mar 2024 09:29:13 +0800 Peng Zhang <zhangpeng362@huawei.com> wrote: > From: ZhangPeng <zhangpeng362@huawei.com> > > The vmf->ptl in filemap_fault_recheck_pte_none() is still set from > handle_pte_fault(). But at the same time, we did a pte_unmap(vmf->pte). > After a pte_unmap(vmf->pte) unmap and rcu_read_unlock(), the page table > may be racily changed and vmf->ptl maybe fails to protect the actual > page table. > Fix this by replacing pte_offset_map() with pte_offset_map_nolock(). > > ... > > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -3207,7 +3207,8 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf) > if (!(vmf->flags & FAULT_FLAG_ORIG_PTE_VALID)) > return 0; > > - ptep = pte_offset_map(vmf->pmd, vmf->address); > + ptep = pte_offset_map_nolock(vma->vm_mm, vmf->pmd, vmf->address, > + &vmf->ptl); > if (unlikely(!ptep)) > return VM_FAULT_NOPAGE; > whoops, I'm still sitting on this because I didn't know whether we should backport it. And... guess what I say next. Can we please describe what are the userspace visible effects of the bug?
On Tue, 25 Jun 2024 14:06:43 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 13 Mar 2024 09:29:13 +0800 Peng Zhang <zhangpeng362@huawei.com> wrote: > > > From: ZhangPeng <zhangpeng362@huawei.com> > > > > The vmf->ptl in filemap_fault_recheck_pte_none() is still set from > > handle_pte_fault(). But at the same time, we did a pte_unmap(vmf->pte). > > After a pte_unmap(vmf->pte) unmap and rcu_read_unlock(), the page table > > may be racily changed and vmf->ptl maybe fails to protect the actual > > page table. > > Fix this by replacing pte_offset_map() with pte_offset_map_nolock(). > > > > ... > > > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -3207,7 +3207,8 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf) > > if (!(vmf->flags & FAULT_FLAG_ORIG_PTE_VALID)) > > return 0; > > > > - ptep = pte_offset_map(vmf->pmd, vmf->address); > > + ptep = pte_offset_map_nolock(vma->vm_mm, vmf->pmd, vmf->address, > > + &vmf->ptl); > > if (unlikely(!ptep)) > > return VM_FAULT_NOPAGE; > > > > whoops, I'm still sitting on this because I didn't know whether we > should backport it. > > And... guess what I say next. Can we please describe what are the > userspace visible effects of the bug? > Nobody? Oh well, I'll add cc:stable amd move this into mm-hotfixes. From: ZhangPeng <zhangpeng362@huawei.com> Subject: filemap: replace pte_offset_map() with pte_offset_map_nolock() Date: Wed, 13 Mar 2024 09:29:13 +0800 The vmf->ptl in filemap_fault_recheck_pte_none() is still set from handle_pte_fault(). But at the same time, we did a pte_unmap(vmf->pte). After a pte_unmap(vmf->pte) unmap and rcu_read_unlock(), the page table may be racily changed and vmf->ptl maybe fails to protect the actual page table. Fix this by replacing pte_offset_map() with pte_offset_map_nolock(). Link: https://lkml.kernel.org/r/20240313012913.2395414-1-zhangpeng362@huawei.com Fixes: 58f327f2ce80 ("filemap: avoid unnecessary major faults in filemap_fault()") Signed-off-by: ZhangPeng <zhangpeng362@huawei.com> Suggested-by: David Hildenbrand <david@redhat.com> Reviewed-by: David Hildenbrand <david@redhat.com> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Cc: "Huang, Ying" <ying.huang@intel.com> Cc: Hugh Dickins <hughd@google.com> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> Cc: Matthew Wilcox (Oracle) <willy@infradead.org> Cc: Nanyong Sun <sunnanyong@huawei.com> Cc: Yang Shi <shy828301@gmail.com> Cc: Yin Fengwei <fengwei.yin@intel.com> Cc: <stable@vger.kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/filemap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- a/mm/filemap.c~filemap-replace-pte_offset_map-with-pte_offset_map_nolock +++ a/mm/filemap.c @@ -3231,7 +3231,8 @@ static vm_fault_t filemap_fault_recheck_ if (!(vmf->flags & FAULT_FLAG_ORIG_PTE_VALID)) return 0; - ptep = pte_offset_map(vmf->pmd, vmf->address); + ptep = pte_offset_map_nolock(vma->vm_mm, vmf->pmd, vmf->address, + &vmf->ptl); if (unlikely(!ptep)) return VM_FAULT_NOPAGE;
On 04.07.24 20:40, Andrew Morton wrote: > On Tue, 25 Jun 2024 14:06:43 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > >> On Wed, 13 Mar 2024 09:29:13 +0800 Peng Zhang <zhangpeng362@huawei.com> wrote: >> >>> From: ZhangPeng <zhangpeng362@huawei.com> >>> >>> The vmf->ptl in filemap_fault_recheck_pte_none() is still set from >>> handle_pte_fault(). But at the same time, we did a pte_unmap(vmf->pte). >>> After a pte_unmap(vmf->pte) unmap and rcu_read_unlock(), the page table >>> may be racily changed and vmf->ptl maybe fails to protect the actual >>> page table. >>> Fix this by replacing pte_offset_map() with pte_offset_map_nolock(). >>> >>> ... >>> >>> --- a/mm/filemap.c >>> +++ b/mm/filemap.c >>> @@ -3207,7 +3207,8 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf) >>> if (!(vmf->flags & FAULT_FLAG_ORIG_PTE_VALID)) >>> return 0; >>> >>> - ptep = pte_offset_map(vmf->pmd, vmf->address); >>> + ptep = pte_offset_map_nolock(vma->vm_mm, vmf->pmd, vmf->address, >>> + &vmf->ptl); >>> if (unlikely(!ptep)) >>> return VM_FAULT_NOPAGE; >>> >> >> whoops, I'm still sitting on this because I didn't know whether we >> should backport it. >> >> And... guess what I say next. Can we please describe what are the >> userspace visible effects of the bug? >> > > Nobody? > > Oh well, I'll add cc:stable amd move this into mm-hotfixes. Yeah, we should better do that. IIRC, the PTL pointer might be stale (use after free?), although I cannot judge how often that should actually happen.
On 2024/7/5 2:40, Andrew Morton wrote: > On Tue, 25 Jun 2024 14:06:43 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > >> On Wed, 13 Mar 2024 09:29:13 +0800 Peng Zhang <zhangpeng362@huawei.com> wrote: >> >>> From: ZhangPeng <zhangpeng362@huawei.com> >>> >>> The vmf->ptl in filemap_fault_recheck_pte_none() is still set from >>> handle_pte_fault(). But at the same time, we did a pte_unmap(vmf->pte). >>> After a pte_unmap(vmf->pte) unmap and rcu_read_unlock(), the page table >>> may be racily changed and vmf->ptl maybe fails to protect the actual >>> page table. >>> Fix this by replacing pte_offset_map() with pte_offset_map_nolock(). >>> >>> ... >>> >>> --- a/mm/filemap.c >>> +++ b/mm/filemap.c >>> @@ -3207,7 +3207,8 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf) >>> if (!(vmf->flags & FAULT_FLAG_ORIG_PTE_VALID)) >>> return 0; >>> >>> - ptep = pte_offset_map(vmf->pmd, vmf->address); >>> + ptep = pte_offset_map_nolock(vma->vm_mm, vmf->pmd, vmf->address, >>> + &vmf->ptl); >>> if (unlikely(!ptep)) >>> return VM_FAULT_NOPAGE; >>> >> whoops, I'm still sitting on this because I didn't know whether we >> should backport it. >> >> And... guess what I say next. Can we please describe what are the >> userspace visible effects of the bug? >> > Nobody? > > Oh well, I'll add cc:stable amd move this into mm-hotfixes. > Sorry for the late reply. As David said, the PTL pointer might be stale so if we continue to use it infilemap_fault_recheck_pte_none(), it might trigger UAF. Also, if the PTL fails, the issue fixed by commit 58f327f2ce80 ("filemap: avoid unnecessary major faults in filemap_fault()") might reappear.
diff --git a/mm/filemap.c b/mm/filemap.c index 31ab455c4537..222adac7c9c5 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3207,7 +3207,8 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf) if (!(vmf->flags & FAULT_FLAG_ORIG_PTE_VALID)) return 0; - ptep = pte_offset_map(vmf->pmd, vmf->address); + ptep = pte_offset_map_nolock(vma->vm_mm, vmf->pmd, vmf->address, + &vmf->ptl); if (unlikely(!ptep)) return VM_FAULT_NOPAGE;