Re: KASAN: use-after-free Read in task_is_descendant

Oleg Nesterov
2018-10-29T12:24:02Z

On 10/26, Kees Cook wrote:
>
> On Thu, Oct 25, 2018 at 2:01 PM, Oleg Nesterov <oleg@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.GA18153@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;