Date:	Fri, 11 Dec 2009 12:25:12 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	tglx@...utronix.de
Cc:	oleg@...hat.com, linux-kernel@...r.kernel.org,
	paulmck@...ux.vnet.ibm.com, linux-security-module@...r.kernel.org
Subject: Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred()access

Thomas Gleixner wrote:
> On Fri, 11 Dec 2009, Tetsuo Handa wrote:
> > > Usually tasklist gives enough protection, but if copy_process() fails
> > > it calls free_pid() lockless and does call_rcu(delayed_put_pid().
> > > This means, without rcu lock find_pid_ns() can't scan the hash table
> > > safely.
> > 
> > So, we need to change below comment from "or" to "and" ?
> 
> No, both functions must be called with rcu_read_lock()
> 
> tasklist_lock read-held is not protecting the rcu lists and does not
> protect against a concurrent update. It merily protects against tasks
> going away or being added while we look up the lists.

So, rcu_read_lock() is mandatory.
But I couldn't understand why tasklist_lock being held is not mandatory.

Caller of these functions will use "struct task_struct" and expect that values
and pointers retrieved by dereferencing returned pointer are valid.

If "struct task_struct" was removed from the list due to missing tasklist_lock
between returning from find_task_by_pid_ns() and dereferencing, I worry that
values and pointers retrieved by dereferencing are invalid.

rcu_read_lock() being held will prevent the returned "struct task_struct" from
being kfree()d, but I think rcu_read_lock() being held does not prevent values
and pointers of "struct task_struct" from being invalidated.



Anyway, I browsed 2.6.32 for find_task_by_vpid() / find_task_by_pid_ns() users
and found below users which lacks read_lock(&tasklist_lock) or rcu_read_lock().

Users missing read_lock(&tasklist_lock) when calling find_task_by_vpid():

  get_net_ns_by_pid() in net/core/net_namespace.c
  seq_print_userip_objs() in kernel/trace/trace_output.c
  fill_pid() in kernel/taskstats.c
  fill_tgid() in kernel/taskstats.c
  futex_find_get_task() in kernel/futex.c
  SYSCALL_DEFINE3(get_robust_list) in kernel/futex.c
  compat_sys_get_robust_list() in kernel/futex_compat.c
  ptrace_get_task_struct() in kernel/ptrace.c
  do_send_specific() in kernel/signal.c
  find_get_context() in kernel/perf_event.c
  posix_cpu_clock_get() in kernel/posix-cpu-timers.c
  good_sigevent() in kernel/posix-timers.c
  attach_task_by_pid() in kernel/cgroup.c
  SYSCALL_DEFINE1(getpgid) in kernel/sys.c
  SYSCALL_DEFINE1(getsid) in kernel/sys.c
  do_sched_setscheduler() in kernel/sched.c

Users missing rcu_read_lock() when calling find_task_by_vpid():

  SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
  SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
  cap_get_target_pid() in kernel/capability.c
  audit_prepare_user_tty() in kernel/audit.c
  audit_receive_msg() in kernel/audit.c
  check_clock() in kernel/posix-cpu-timers.c
  posix_cpu_timer_create() in kernel/posix-cpu-timers.c
  SYSCALL_DEFINE3(setpriority) in kernel/sys.c
  SYSCALL_DEFINE2(getpriority) in kernel/sys.c
  SYSCALL_DEFINE2(setpgid) in kernel/sys.c
  SYSCALL_DEFINE1(sched_getscheduler) in kernel/sched.c
  SYSCALL_DEFINE2(sched_getparam) in kernel/sched.c
  sched_setaffinity() in kernel/sched.c
  sched_getaffinity() in kernel/sched.c
  SYSCALL_DEFINE2(sched_rr_get_interval) in kernel/sched.c
  tomoyo_is_select_one() in security/tomoyo/common.c
  tomoyo_read_pid() in security/tomoyo/common.c
  SYSCALL_DEFINE6(move_pages) in mm/migrate.c
  SYSCALL_DEFINE4(migrate_pages) in mm/mempolicy.c
  find_process_by_pid() in arch/mips/kernel/mips-mt-fpaff.c
  pfm_get_task() in arch/ia64/kernel/perfmon.c
  cxn_pin_by_pid() in arch/frv/mm/mmu-context.c

Users missing read_lock(&tasklist_lock) when calling find_task_by_pid_ns():

  rest_init() in init/main.c
  proc_pid_lookup() in fs/proc/base.c
  proc_task_lookup() in fs/proc/base.c
  first_tid() in fs/proc/base.c
  getthread() in kernel/kgdb.c
  mconsole_stack() in arch/um/drivers/mconsole_kern.c

Users missing rcu_read_lock() when calling find_task_by_pid_ns():

  rest_init() in init/main.c
  getthread() in kernel/kgdb.c
  mconsole_stack() in arch/um/drivers/mconsole_kern.c

Regarding users which lack rcu_read_lock(), users call
read_lock(&tasklist_lock) in order to access "struct task_struct" returned
by find_task_by_pid_ns() but they do not want to be bothered by internal pid
structure. Thus, the fix would be to add rcu_read_lock() and rcu_read_unlock()
inside find_task_by_pid_ns().

But how to check users which lack read_lock(&tasklist_lock) but expecting that
it is safe to access "struct task_struct" returned by find_task_by_pid_ns() ?
--
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