Messages in this thread | | | Subject | Re: [patch 04/12] stackdepot: fix mempolicy use-after-free | From | Andrey Ryabinin <> | Date | Wed, 31 Aug 2016 19:16:44 +0300 |
| |
On 08/31/2016 12:52 AM, David Rientjes wrote: > On Fri, 26 Aug 2016, Vegard Nossum wrote: >
[...]
>> >> If nobody beats me to it, I can submit a new patch. The reason I haven't >> submitted it just yet is because I want a chance to test that I get it >> right and that the bug goes away. In the meantime this patch does fix a >> bug that I ran into frequently. I don't think the patch above is total >> crap. >> > > What we want to do is stash away current->mempolicy, do > current->mempolicy = NULL so functions like alloc_pages() don't look at > it, and then do mpol_put() on it. task_unlock() acts a memory barrier in > this case, lib/stackdepot.c needs no change. > > I think we need this backported to stable for 4.5+. >
4.6+, as this fixes: cd11016e5f52 ("mm, kasan: stackdepot implementation. Enable stackdepot for SLAB")
$ git describe --contains cd11016e5f5212c v4.6-rc1~6^2~4
> Whaddyathink? >
Acked-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> --->o--- > > mm, mempolicy: task->mempolicy must be NULL before dropping final reference > > KASAN allocates memory from the page allocator as part of > kmem_cache_free(), and that can reference current->mempolicy through any > number of allocation functions. It needs to be NULL'd out before the > final reference is dropped to prevent a use-after-free bug: > > BUG: KASAN: use-after-free in alloc_pages_current+0x363/0x370 at addr ffff88010b48102c > CPU: 0 PID: 15425 Comm: trinity-c2 Not tainted 4.8.0-rc2+ #140 > ... > Call Trace: > dump_stack > kasan_object_err > kasan_report_error > __asan_report_load2_noabort > alloc_pages_current <-- use after free > depot_save_stack > save_stack > kasan_slab_free > kmem_cache_free > __mpol_put <-- free > do_exit > > This patch sets current->mempolicy to NULL before dropping the final > reference. > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > Signed-off-by: David Rientjes <rientjes@google.com> > --- > include/linux/mempolicy.h | 4 ++++ > kernel/exit.c | 7 +------ > mm/mempolicy.c | 17 +++++++++++++++++ > 3 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h > --- a/include/linux/mempolicy.h > +++ b/include/linux/mempolicy.h > @@ -195,6 +195,7 @@ static inline bool vma_migratable(struct vm_area_struct *vma) > } > > extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); > +extern void mpol_put_task_policy(struct task_struct *); > > #else > > @@ -297,5 +298,8 @@ static inline int mpol_misplaced(struct page *page, struct vm_area_struct *vma, > return -1; /* no node preference */ > } > > +static inline void mpol_put_task_policy(struct task_struct *task) > +{ > +} > #endif /* CONFIG_NUMA */ > #endif > diff --git a/kernel/exit.c b/kernel/exit.c > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -848,12 +848,7 @@ void do_exit(long code) > TASKS_RCU(preempt_enable()); > exit_notify(tsk, group_dead); > proc_exit_connector(tsk); > -#ifdef CONFIG_NUMA > - task_lock(tsk); > - mpol_put(tsk->mempolicy); > - tsk->mempolicy = NULL; > - task_unlock(tsk); > -#endif > + mpol_put_task_policy(tsk); > #ifdef CONFIG_FUTEX > if (unlikely(current->pi_state_cache)) > kfree(current->pi_state_cache); > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -2336,6 +2336,23 @@ out: > return ret; > } > > +/* > + * Drop the (possibly final) reference to task->mempolicy. It needs to be > + * dropped after task->mempolicy is set to NULL so that any allocation done as > + * part of its kmem_cache_free(), such as by KASAN, doesn't reference a freed > + * policy. > + */ > +void mpol_put_task_policy(struct task_struct *task) > +{ > + struct mempolicy *pol; > + > + task_lock(task); > + pol = task->mempolicy; > + task->mempolicy = NULL; > + task_unlock(task); > + mpol_put(pol); > +} > + > static void sp_delete(struct shared_policy *sp, struct sp_node *n) > { > pr_debug("deleting %lx-l%lx\n", n->start, n->end); >
| |