Re: [patch 04/12] stackdepot: fix mempolicy use-after-free

[Andrey Ryabinin]

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@xxxxxxxxxxxxx>

> --->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@xxxxxxxxxx>
> Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx>
> ---
>  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);
>