Messages in this thread | | | Date | Thu, 7 May 2009 08:36:06 +0200 | From | Oleg Nesterov <> | Subject | Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access() |
| |
On 05/06, Roland McGrath wrote: > > > I was going to cleanup this later. Because I think that > > __ptrace_may_access() should die. It has only one caller, mm_for_maps(). > > CC'ing Al Viro, who wrote mm_for_maps() (and no one has touched it since, > see commit 831830b). > > > I will re-check, but it looks a bit strange. More precisely, I just > > can't understand it. Why we can't just do > > > > struct mm_struct *mm_for_maps(struct task_struct *task) > > { > > struct mm_struct *mm = get_task_mm(task); > > > > if (mm && mm != current->mm && !ptrace_may_access()) { > > mmput(mm); > > mm = NULL; > > } > > > > return mm; > > } > > That seems fine to me. I suspect the old code just predated the PF_KTHREAD > check in get_task_mm() and excluding the borrowed-mm window races was the > only reason for using task_lock() that way. > > > ? We do not care if this task exits and clears ->mm right before > > or after ptrace_may_access(), and this is possible eith the current > > code too once it drops tasklist. > > I agree.
Great. Will try to make the patches soon.
And I forgot to mention, there is another reason to kill __ptrace_may_access. Because we can "narrow" the critical section protected by task_lock(). Not for performance of course, just for clarity:
/* the callers of ptrace_may_access should be fixed */
int ptrace_may_access(struct task_struct *task, unsigned int mode) { const struct cred *cred = current_cred(), *tcred; int ret = 0;
/* May we inspect the given task? * This check is used both for attaching with ptrace * and for allowing access to sensitive information in /proc. * * ptrace_attach denies several cases that /proc allows * because setting up the necessary parent/child relationship * or halting the specified task is impossible. */ /* Don't let security modules deny introspection */ if (task == current) return ret;
rcu_read_lock(); tcred = __task_cred(task); if ((cred->uid != tcred->euid || cred->uid != tcred->suid || cred->uid != tcred->uid || cred->gid != tcred->egid || cred->gid != tcred->sgid || cred->gid != tcred->gid) && !capable(CAP_SYS_PTRACE)) ret = -EPERM; rcu_read_unlock(); if (ret) return ret; /* kill rmb ? */
task_lock(task); if (!task->mm || !get_dumpable(task->mm)) { if (!capable(CAP_SYS_PTRACE)) ret = -EPERM; task_unclock(task); if (ret) return ret;
return security_ptrace_may_access(task, mode); }
Btw, "[PATCH 3/3]" notes that security_ptrace_may_access() is called without task_lock(), this note "leaked" from this change in future ;)
But firsty I'll try to grep/recheck this all.
Oleg.
| |