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, 14 Jul 2014 19:31:26 +0400
From:	Andrey Ryabinin <a.ryabinin@...sung.com>
To:	Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>
Cc:	Sasha Levin <sasha.levin@...cle.com>,
	Ingo Molnar <mingo@...nel.org>,
	John Stultz <john.stultz@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Frederic Weisbecker <fweisbec@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Dave Jones <davej@...hat.com>
Subject: Re: sched, timers: use after free in __lock_task_sighand when	exiting
 a process

On 07/14/14 18:49, Oleg Nesterov wrote:
> On 07/14, Peter Zijlstra wrote:
>>
>> On Sun, Jul 13, 2014 at 07:45:56PM -0400, Sasha Levin wrote:
>>>
>>> [  876.319044] ==================================================================
>>> [  876.319044] AddressSanitizer: use after free in do_raw_spin_unlock+0x4b/0x1a0 at addr ffff8803e48cec18
>>> [  876.319044] page:ffffea000f923380 count:0 mapcount:0 mapping:          (null) index:0x0
>>> [  876.319044] page flags: 0x2fffff80008000(tail)
>>> [  876.319044] page dumped because: kasan error
>>> [  876.319044] CPU: 26 PID: 8749 Comm: trinity-watchdo Tainted: G        W      3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #817
>>> [  876.319044]  00000000000000fb 0000000000000000 ffffea000f923380 ffff8805c417fc70
>>> [  876.319044]  ffffffff9de47068 ffff8805c417fd40 ffff8805c417fd30 ffffffff99426f5c
>>> [  876.319044]  0000000000000010 0000000000000000 ffff8805c417fc9d 66666620000000a8
>>> [  876.319044] Call Trace:
>>> [  876.319044] dump_stack (lib/dump_stack.c:52)
>>> [  876.319044] kasan_report_error (mm/kasan/report.c:98 mm/kasan/report.c:166)
>>> [  876.319044] __asan_load8 (mm/kasan/kasan.c:364)
>>> [  876.319044] do_raw_spin_unlock (./arch/x86/include/asm/current.h:14 kernel/locking/spinlock_debug.c:99 kernel/locking/spinlock_debug.c:158)
>>> [  876.319044] _raw_spin_unlock (include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:183)
>>> [  876.319044] __lock_task_sighand (include/linux/rcupdate.h:858 kernel/signal.c:1285)
>>> [  876.319044] do_send_sig_info (kernel/signal.c:1191)
>>> [  876.319044] group_send_sig_info (kernel/signal.c:1304)
>>> [  876.319044] kill_pid_info (kernel/signal.c:1339)
>>> [  876.319044] SYSC_kill (kernel/signal.c:1423 kernel/signal.c:2900)
> 
> Looks like a false alarm at first glance...
> 
>> Oleg, what guarantees the RCU free of task-struct and sighand?
> 
>> The only RCU I can find is delayed_put_task_struct() but that's not
>> often used.
> 
> Yes, usually the code uses put_task_struct(). delayed_put_task_struct()
> acts almost as "if (dec_and_test(usage)) kfree_rcu(), but allows to use
> get_task_struct() if you observe this task under rcu_read_lock().
> 
> Say,
> 	rcu_read_lock();
> 	task = find_task_by_vpid(...);
> 	if (task)
> 		get_task_struct(task);
> 	rcu_read_unlock();
> 
> If release_task() used dec_and_test + kfree_rcu, the code above could
> not work.
> 
>> TASK_DEAD etc. use regular put_task_struct() and that
>> doesn't seem to involve RCU.
> 
> Yes, the task itself (or, depending ob pov, scheduler) has a reference.
> copy_process() does
> 
> 	/*
> 	 * One for us, one for whoever does the "release_task()" (usually
> 	 * parent)
> 	 */
> 	atomic_set(&tsk->usage, 2);
> 
> "us" actually means that put_task_struct(TASK_DEAD).
> 
> As for ->sighand, note that sighand_cachep is SLAB_DESTROY_BY_RCU. So this
> memory is RCU free in a sense that it can't be returned to system, but it
> can be reused by another task. This is fine, lock_task_sighand() rechecks
> sighand == task->sighand under ->siglock.
> 
> So perhaps this tool misinterprets kmem_cache_free(sighand_cachep) as use
> after free?
> 

Yes, you are right. I already seen such false positive with rcu slabs and kasan,
and it was fixed at some point, but somehow fix for this didn't get to the final patches.

I'm sorry for wasting everyone's time.

> We are going to add some comments into lock_task_sighand(). And cleanup it,
> it can look much simpler.
> 
> Oleg.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists