linux-kernel - Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20131220175926.GA31032@redhat.com>
Date:	Fri, 20 Dec 2013 18:59:26 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Richard Guy Briggs <rgb@...hat.com>
Cc:	John Johansen <john.johansen@...onical.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Eric Paris <eparis@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] apparmor: remove the "task" arg from
	may_change_ptraced_domain()

On 12/19, Richard Guy Briggs wrote:
>
> On 13/12/18, Oleg Nesterov wrote:
>
> > Otherwise I can't understand your email, at least right now... I do not
> > know how/where audit uses parent/real_parent.
>
> It uses real_parent to include the ppid number of a process in a couple
> of log records.

I did a quick grep, it seems that audit uses sys_getppid() which should be
fine. Nevermind, if you meant that (say) audit_alloc() paths use tsk->parent
somehow, I agree this doesn't look right/safe. new_child->*parent can point
to nowhere right after dup_task_struct() and there is no way to detect this.

Unless, of course new_child->*parent == current, but copy_process() changes
child->parent only after it takes tasklist_lock.

> > But yes, unless tsk == current, the usage of tsk->*parent is not safe even
> > under rcu_read_lock() unless you verify that this task was not unhashed.
>
> As I said, the only case I can see is in copy_process() when a signal is
> pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it.
> Still, that is enough to need to check it.

Hmm, so I guess you are worried about audit_free?

But this error path can be called even before it checks signal_pending(),
suppose that copy_semundo() fails?

So it seems that CLONE_PARENT/THREAD doesn't really matter because it
audit_free() can be called before copy_process() sets ->parent = current?

Most probably I misunderstood you, so please ignore. I am sure you fully
understand the problems and do not need my comments ;)

> So what are you saying?  I should use pid_alive() inside the
> rcu_read_lock() to verify it is not unhashed since I don't have anything
> to do with ->ptrace or any other task lock?  In fact, the answer is
> under my nose in __task_pid_nr_ns(), which already uses this approach.

Yes. task->parent (or real_parent) can only make sense if this task can
be re-parented (if parent exits, or debugger detaches). If the task is
dead (removed from the parent->children list), obviously nobody can update
this pointer.

And this reminds me we should simply clear ->parent/real_parent on exit.
And ->group_leader. I'll try to make the patch(s), after I finish
thread_group changes. Unfortunately this needs a lot of grepping.

Oleg.

--
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/