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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 30 Nov 2020 22:57:54 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Axel Rasmussen <axelrasmussen@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Chinwen Chang <chinwen.chang@...iatek.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        David Rientjes <rientjes@...gle.com>,
        Davidlohr Bueso <dbueso@...e.de>,
        Ingo Molnar <mingo@...hat.com>, Jann Horn <jannh@...gle.com>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        Michel Lespinasse <walken@...gle.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Vlastimil Babka <vbabka@...e.cz>,
        Yafang Shao <laoar.shao@...il.com>, davem@...emloft.net,
        dsahern@...nel.org, gregkh@...uxfoundation.org, kuba@...nel.org,
        liuhangbin@...il.com, tj@...nel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH] mm: mmap_lock: fix use-after-free race and css ref leak
 in tracepoints

On Mon, 30 Nov 2020 15:35:04 -0800
Axel Rasmussen <axelrasmussen@...gle.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@...gle.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