| Message ID | 20230726214103.3261108-3-jannh@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fix vma->anon_vma check for per-VMA locking; fix anon_vma memory ordering | expand |
On Wed, Jul 26, 2023 at 2:42 PM Jann Horn <jannh@google.com> wrote: > > lock_vma_under_rcu() tries to guarantee that __anon_vma_prepare() can't > be called in the VMA-locked page fault path by ensuring that > vma->anon_vma is set. > However, this check happens before the VMA is locked, which means a > concurrent move_vma() can concurrently call unlink_anon_vmas(), which > disassociates the VMA's anon_vma. > > This means we can get UAF in the following scenario: > > THREAD 1 THREAD 2 > ======== ======== > <page fault> > lock_vma_under_rcu() > rcu_read_lock() > mas_walk() > check vma->anon_vma > > mremap() syscall > move_vma() > vma_start_write() > unlink_anon_vmas() > <syscall end> > > handle_mm_fault() > __handle_mm_fault() > handle_pte_fault() > do_pte_missing() > do_anonymous_page() > anon_vma_prepare() > __anon_vma_prepare() > find_mergeable_anon_vma() > mas_walk() [looks up VMA X] > > munmap() syscall (deletes VMA X) > > reusable_anon_vma() [called on freed VMA X] > > This is a security bug if you can hit it, although an attacker would > have to win two races at once where the first race window is only a > few instructions wide. > > This patch is based on some previous discussion with Linus Torvalds on > the security list. > > Cc: stable@vger.kernel.org > Fixes: 5e31275cc997 ("mm: add per-VMA lock and helper functions to control it") > Signed-off-by: Jann Horn <jannh@google.com> Reviewed-by: Suren Baghdasaryan <surenb@google.com> > --- > mm/memory.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index 01f39e8144ef..603b2f419948 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -5393,27 +5393,28 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, > if (!vma_is_anonymous(vma) && !vma_is_tcp(vma)) > goto inval; > > - /* find_mergeable_anon_vma uses adjacent vmas which are not locked */ > - if (!vma->anon_vma && !vma_is_tcp(vma)) > - goto inval; > - > if (!vma_start_read(vma)) > goto inval; > > + /* > + * find_mergeable_anon_vma uses adjacent vmas which are not locked. > + * This check must happen after vma_start_read(); otherwise, a > + * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA > + * from its anon_vma. > + */ > + if (unlikely(!vma->anon_vma && !vma_is_tcp(vma))) > + goto inval_end_read; > + > /* > * Due to the possibility of userfault handler dropping mmap_lock, avoid > * it for now and fall back to page fault handling under mmap_lock. > */ > - if (userfaultfd_armed(vma)) { > - vma_end_read(vma); > - goto inval; > - } > + if (userfaultfd_armed(vma)) > + goto inval_end_read; > > /* Check since vm_start/vm_end might change before we lock the VMA */ > - if (unlikely(address < vma->vm_start || address >= vma->vm_end)) { > - vma_end_read(vma); > - goto inval; > - } > + if (unlikely(address < vma->vm_start || address >= vma->vm_end)) > + goto inval_end_read; > > /* Check if the VMA got isolated after we found it */ > if (vma->detached) { > @@ -5425,6 +5426,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, > > rcu_read_unlock(); > return vma; > + > +inval_end_read: > + vma_end_read(vma); > inval: > rcu_read_unlock(); > count_vm_vma_lock_event(VMA_LOCK_ABORT); > -- > 2.41.0.487.g6d72f3e995-goog >
diff --git a/mm/memory.c b/mm/memory.c index 01f39e8144ef..603b2f419948 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -5393,27 +5393,28 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, if (!vma_is_anonymous(vma) && !vma_is_tcp(vma)) goto inval; - /* find_mergeable_anon_vma uses adjacent vmas which are not locked */ - if (!vma->anon_vma && !vma_is_tcp(vma)) - goto inval; - if (!vma_start_read(vma)) goto inval; + /* + * find_mergeable_anon_vma uses adjacent vmas which are not locked. + * This check must happen after vma_start_read(); otherwise, a + * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA + * from its anon_vma. + */ + if (unlikely(!vma->anon_vma && !vma_is_tcp(vma))) + goto inval_end_read; + /* * Due to the possibility of userfault handler dropping mmap_lock, avoid * it for now and fall back to page fault handling under mmap_lock. */ - if (userfaultfd_armed(vma)) { - vma_end_read(vma); - goto inval; - } + if (userfaultfd_armed(vma)) + goto inval_end_read; /* Check since vm_start/vm_end might change before we lock the VMA */ - if (unlikely(address < vma->vm_start || address >= vma->vm_end)) { - vma_end_read(vma); - goto inval; - } + if (unlikely(address < vma->vm_start || address >= vma->vm_end)) + goto inval_end_read; /* Check if the VMA got isolated after we found it */ if (vma->detached) { @@ -5425,6 +5426,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, rcu_read_unlock(); return vma; + +inval_end_read: + vma_end_read(vma); inval: rcu_read_unlock(); count_vm_vma_lock_event(VMA_LOCK_ABORT);
lock_vma_under_rcu() tries to guarantee that __anon_vma_prepare() can't be called in the VMA-locked page fault path by ensuring that vma->anon_vma is set. However, this check happens before the VMA is locked, which means a concurrent move_vma() can concurrently call unlink_anon_vmas(), which disassociates the VMA's anon_vma. This means we can get UAF in the following scenario: THREAD 1 THREAD 2 ======== ======== <page fault> lock_vma_under_rcu() rcu_read_lock() mas_walk() check vma->anon_vma mremap() syscall move_vma() vma_start_write() unlink_anon_vmas() <syscall end> handle_mm_fault() __handle_mm_fault() handle_pte_fault() do_pte_missing() do_anonymous_page() anon_vma_prepare() __anon_vma_prepare() find_mergeable_anon_vma() mas_walk() [looks up VMA X] munmap() syscall (deletes VMA X) reusable_anon_vma() [called on freed VMA X] This is a security bug if you can hit it, although an attacker would have to win two races at once where the first race window is only a few instructions wide. This patch is based on some previous discussion with Linus Torvalds on the security list. Cc: stable@vger.kernel.org Fixes: 5e31275cc997 ("mm: add per-VMA lock and helper functions to control it") Signed-off-by: Jann Horn <jannh@google.com> --- mm/memory.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)