Re: sched, timers: use after free in __lock_task_sighand when exiting a process

[Andrey Ryabinin <a.ryabinin@samsung.com>]

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.
> 
>