Re: [PATCH] mm: mmap_lock: fix use-after-free race and css ref leak in tracepoints

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 30 Nov 2020 15:35:04 -0800
Axel Rasmussen <axelrasmussen@google.com> wrote:

> syzbot reported[1] a use-after-free introduced in 0f818c4bc1f3. The bug
> is that an ongoing trace event might race with the tracepoint being
> disabled (and therefore the _unreg() callback being called). Consider
> this ordering:
> 
> T1: trace event fires, get_mm_memcg_path() is called
> T1: get_memcg_path_buf() returns a buffer pointer
> T2: trace_mmap_lock_unreg() is called, buffers are freed
> T1: cgroup_path() is called with the now-freed buffer
> 
> The solution in this commit is to modify trace_mmap_lock_unreg() to
> first stop new buffers from being handed out, and then to wait (spin)
> until any existing buffer references are dropped (i.e., those trace
> events complete).
> 
> I have a simple reproducer program which spins up two pools of threads,
> doing the following in a tight loop:
> 
>   Pool 1:
>   mmap(NULL, 4096, PROT_READ | PROT_WRITE,
>        MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)
>   munmap()
> 
>   Pool 2:
>   echo 1 > /sys/kernel/debug/tracing/events/mmap_lock/enable
>   echo 0 > /sys/kernel/debug/tracing/events/mmap_lock/enable
> 
> This triggers the use-after-free very quickly. With this patch, I let it
> run for an hour without any BUGs.
> 
> While fixing this, I also noticed and fixed a css ref leak. Previously
> we called get_mem_cgroup_from_mm(), but we never called css_put() to
> release that reference. get_mm_memcg_path() now does this properly.
> 
> [1]: https://syzkaller.appspot.com/bug?extid=19e6dd9943972fa1c58a
> 
> Fixes: 0f818c4bc1f3 ("mm: mmap_lock: add tracepoints around lock acquisition")
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
>

Looking at the original patch that this fixes, I'm thinking, why use a
spinlock in the reg/unreg callers? Registering and unregistering a
tracepoint can sleep (it calls mutex locks), so the reg/unreg can sleep
too. As the use of the get_mm_memcg_path() is done under
preempt_disable, which is a rcu grace period, you could simply change
the unregister to:

void trace_mmap_lock_unreg(void)
{
	int cpu;

	mutex_lock(&reg_lock);

	if (--reg_refcount)
		goto out;

	/* Make sure all users of memcg_path_buf are done */
	synchronize_rcu();

	for_each_possible_cpu(cpu) {
		kfree(per_cpu(memcg_path_buf, cpu));
	}

out:
	mutex_unlock(&reg_lock);
}


Obviously, you would need to change reg_lock to mutex in the _reg()
function.

-- Steve