On 10/26, Kees Cook wrote:
>
> On Thu, Oct 25, 2018 at 2:01 PM, Oleg Nesterov <o...@redhat.com> wrote:
> > On 10/25, Oleg Nesterov wrote:
> >> perhaps it needs some changes too. I even have a vague feeling that I have
> >> already
> >> blamed this function some time ago...
> >
> > Heh, yes, 3 years ago ;)
> >
> > https://lore.kernel.org/lkml/20150106184427.ga18...@redhat.com/
> >
> > I can't understand my email today, but note that I tried to point out that
> > task_is_descendant() can dereference the freed mem.
>
> Instead of:
>
> while (walker->pid > 0) {
>
> should it simply be "while (pid_liave(walker)) {"?
No, this would be wrong. Probably walker->pid > 0 is not the best check,
but we do not need to change it for correctness.
> And add a
> pid_alive(parent) after rcu_read_lock()?
So you too do not read my emails ;)
I still think we need a single pid_alive() check and I even sent the patch.
Attached again at the end.
To clarify, let me repeat that ptracer_exception_found() may need some fixes
too, right now I am only talking about task_is_descendant().
> > And yes, task_is_descendant() is overcompicated for no reason, afaics.
>
> Yeah, agreed. I'll fix this up.
I have already posted this code, this is what I think it should do.
static int task_is_descendant(struct task_struct *parent,
struct task_struct *child)
{
struct task_struct *walker;
for (walker = child; walker->pid; walker =
rcu_dereference(walker->real_parent)) {
if (same_thread_group(parent, walker))
return 1;
}
return 0;
}
This version differs in that I removed the parent/child != NULL at the start
and rcu_read_lock(), it should be held by the caller anyway.
> Just to make sure I'm not crazy: the
> real_parent of all tasks in a thread group are the same, yes?
Well, yes and no. So if
same_thread_group(t1, t2) == T
then
same_thread_group(t1->real_parent, t2->real_parent) == T
which means that real_parent of all tasks in a thread group is the same
_process_.
But t1->real_parent and t2->real_parent are not necessarily the same task.
Oleg.
--- x/security/yama/yama_lsm.c
+++ x/security/yama/yama_lsm.c
@@ -368,7 +368,8 @@ static int yama_ptrace_access_check(stru
break;
case YAMA_SCOPE_RELATIONAL:
rcu_read_lock();
- if (!task_is_descendant(current, child) &&
+ if (!pid_alive(child) ||
+ !task_is_descendant(current, child) &&
!ptracer_exception_found(current, child) &&
!ns_capable(__task_cred(child)->user_ns,
CAP_SYS_PTRACE))
rc = -EPERM;
