This cleans up the ancestry check by separating it out into its own
function, which makes the code a bit more readable. Additionally, it
now checks for and uses the thread group leader since otherwise we may
end up missing potential matches on attaches to threads.
Signed-off-by: Kees Cook <[email protected]>
---
security/yama/yama_lsm.c | 55 ++++++++++++++++++++++++++++++++-------------
1 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 72929d2..3b76386 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -21,6 +21,40 @@ static int protected_sticky_symlinks = 1;
static int protected_nonaccess_hardlinks = 1;
/**
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+static int task_is_descendant(struct task_struct *parent,
+ struct task_struct *child)
+{
+ int rc = 0;
+ struct task_struct *walker = child;
+
+ if (!parent || !child)
+ return 0;
+
+ rcu_read_lock();
+ read_lock(&tasklist_lock);
+ while (walker->pid > 0) {
+ if (!thread_group_leader(walker))
+ walker = walker->group_leader;
+ if (walker == parent) {
+ rc = 1;
+ break;
+ }
+ walker = walker->real_parent;
+ }
+ read_unlock(&tasklist_lock);
+ rcu_read_unlock();
+
+ return rc;
+}
+
+
+/**
* yama_ptrace_access_check - validate PTRACE_ATTACH calls
* @child: child task pointer
* @mode: ptrace attach mode
@@ -37,22 +71,11 @@ static int yama_ptrace_access_check(struct task_struct *child,
return rc;
/* require ptrace target be a child of ptracer on attach */
- if (mode == PTRACE_MODE_ATTACH && ptrace_scope &&
- !capable(CAP_SYS_PTRACE)) {
- struct task_struct *walker = child;
-
- rcu_read_lock();
- read_lock(&tasklist_lock);
- while (walker->pid > 0) {
- if (walker == current)
- break;
- walker = walker->real_parent;
- }
- if (walker->pid == 0)
- rc = -EPERM;
- read_unlock(&tasklist_lock);
- rcu_read_unlock();
- }
+ if (mode == PTRACE_MODE_ATTACH &&
+ ptrace_scope &&
+ !task_is_descendant(current, child) &&
+ !capable(CAP_SYS_PTRACE))
+ rc = -EPERM;
if (rc) {
char name[sizeof(current->comm)];
--
1.7.1
--
Kees Cook
Ubuntu Security Team
Kees Cook wrote:
> +static int task_is_descendant(struct task_struct *parent,
> + struct task_struct *child)
> +{
> + int rc = 0;
> + struct task_struct *walker = child;
> +
> + if (!parent || !child)
> + return 0;
parent (== current) is !NULL and
child (in original code) is !NULL.
You can remove this check unless you are planning to call
this function from other places.
> + if (mode == PTRACE_MODE_ATTACH &&
> + ptrace_scope &&
> + !task_is_descendant(current, child) &&
> + !capable(CAP_SYS_PTRACE))
> + rc = -EPERM;
I don't know how heavy capable(CAP_SYS_PTRACE) is.
But checking !capable(CAP_SYS_PTRACE) before
!task_is_descendant(current, child) might be lighter.
On Wed, Jul 14, 2010 at 09:19:09AM +0900, Tetsuo Handa wrote:
> Kees Cook wrote:
> > +static int task_is_descendant(struct task_struct *parent,
> > + struct task_struct *child)
> > +{
> > + int rc = 0;
> > + struct task_struct *walker = child;
> > +
> > + if (!parent || !child)
> > + return 0;
>
> parent (== current) is !NULL and
> child (in original code) is !NULL.
> You can remove this check unless you are planning to call
> this function from other places.
I'd like the flexibility to call it with NULLs. But yes, at present, it
never will be NULL.
> > + if (mode == PTRACE_MODE_ATTACH &&
> > + ptrace_scope &&
> > + !task_is_descendant(current, child) &&
> > + !capable(CAP_SYS_PTRACE))
> > + rc = -EPERM;
>
> I don't know how heavy capable(CAP_SYS_PTRACE) is.
> But checking !capable(CAP_SYS_PTRACE) before
> !task_is_descendant(current, child) might be lighter.
That's the order I had before, but in looking at some of the other code, it
seemed like moving it to the end made more logical sense. Since checking
PTRACE attach isn't a common or time-sensitive operation, I figured trying
to tune it wasn't critical.
-Kees
--
Kees Cook
Ubuntu Security Team
Quoting Kees Cook ([email protected]):
> On Wed, Jul 14, 2010 at 09:19:09AM +0900, Tetsuo Handa wrote:
> > > + if (mode == PTRACE_MODE_ATTACH &&
> > > + ptrace_scope &&
> > > + !task_is_descendant(current, child) &&
> > > + !capable(CAP_SYS_PTRACE))
> > > + rc = -EPERM;
> >
> > I don't know how heavy capable(CAP_SYS_PTRACE) is.
> > But checking !capable(CAP_SYS_PTRACE) before
> > !task_is_descendant(current, child) might be lighter.
>
> That's the order I had before, but in looking at some of the other code, it
> seemed like moving it to the end made more logical sense. Since checking
> PTRACE attach isn't a common or time-sensitive operation, I figured trying
> to tune it wasn't critical.
Yes the reason to keep it like this is that capable(CAP_SYS_PTRACE)
will set PF_SUPERPRIV if it passes. You don't want to do that unless
the capability was actually required.
-serge
On Tue, 13 Jul 2010, Kees Cook wrote:
> This cleans up the ancestry check by separating it out into its own
> function, which makes the code a bit more readable. Additionally, it
> now checks for and uses the thread group leader since otherwise we may
> end up missing potential matches on attaches to threads.
>
> Signed-off-by: Kees Cook <[email protected]>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
--
James Morris
<[email protected]>