Re: [PATCH] apparmor: remove the "task" arg frommay_change_ptraced_domain()

[Richard Guy Briggs]

On 13/12/18, Oleg Nesterov wrote:
> On 12/18, Richard Guy Briggs wrote:
> >
> > Bcc: rgb@xxxxxxxxxx
> > Subject: Re: [PATCH] apparmor: remove the "task" arg from
> >  may_change_ptraced_domain()
> > Reply-To:
> > In-Reply-To: <20130926132519.GY13968@xxxxxxxxxxxxxxxxxxxx>
> 
> The subject is empty ;) I changed it to match the above.

HTH?!?  Thanks for adding it.  (more below...)

> > On 13/09/26, Richard Guy Briggs wrote:
> > > On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote:
> > > > On 09/23, Richard Guy Briggs wrote:
> > > > >
> > > > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> > > > > > Unless task == current ptrace_parent(task) is not safe even under
> > > > > > rcu_read_lock() and most of the current users are not right.
> > > > >
> > > > > Could you point to an explanation of this?
> > > >
> > > > If this task exits before rcu_read_lock() ->parent can point to the
> > > > already freed/reused memory.
> > >
> > > Ok, understood.  So even though the task may have exited, the task
> > > struct pointer is still valid, but not the contents of the task struct
> > > to which it points.
> >
> > [The thread also relates to the patch
> > 	"pid: get ppid pid_t of task in init_pid_ns safely"
> > in which sys_getppid() (which appears safe) is replaced with something that
> > references the init_pid_ns rather than current's pid_ns.]
> >
> > So, in the general case, that call is not safe, and we should at least
> > remove the task_struct argument.
> 
> I changed my mind, please see the recent discussion with Paul:
> 
> 	http://marc.info/?t=138626281900001
> 
> instead we should document why ptrace_parent() is safe without pid_alive().

Interesting.  I wasn't aware of pid_alive(), but that would certainly
help.

> I hope that the change in apparmor was fine anyway.

Yes, I'm fine with apparmor change, if it was deemed that the ppid
wasn't needed.  If it is, then it should use this new task_ppid_nr().
Better yet I think to generalize it to anticipate auditd in containers.

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

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

> ptrace_parent() is safe because it checks ->ptrace. Previously I thought
> we should not rely on this, but the additional pid_alive() looks ugly so
> it would be better to simply document this. I'll send the patch.

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.
And rcu_read_lock() can be nested.

> Oleg.

- RGB

--
Richard Guy Briggs <rbriggs@xxxxxxxxxx>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/