linux-kernel - Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task->pid and task->tgid

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130827023722.GB21098@madcap2.tricolour.ca>
Date:	Mon, 26 Aug 2013 22:37:22 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Oleg Nesterov <oleg@...hat.com>, linux-kernel@...r.kernel.org,
	linux-audit@...hat.com,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Eric Paris <eparis@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...hat.com>,
	"Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH 11/12] pid: rewrite task helper functions avoiding
 task->pid and task->tgid

On Fri, Aug 23, 2013 at 08:36:21AM +0200, Peter Zijlstra wrote:
> On Thu, Aug 22, 2013 at 05:43:47PM -0400, Richard Guy Briggs wrote:
> > On Thu, Aug 22, 2013 at 10:05:55PM +0200, Peter Zijlstra wrote:
> > > On Tue, Aug 20, 2013 at 05:32:03PM -0400, Richard Guy Briggs wrote:
> > > > This stops these four task helper functions from using the deprecated and
> > > > error-prone task->pid and task->tgid.
> > > > 
> > > > (informed by ebiederman's ea5a4d01)
> > > > Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
> > > > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > > > ---
> > > >  include/linux/sched.h |    8 ++++----
> > > >  1 files changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index 8e69807..46e739d 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -1579,7 +1579,7 @@ static inline int pid_alive(struct task_struct *p)
> > > >   */
> > > >  static inline int is_global_init(struct task_struct *tsk)
> > > >  {
> > > > -	return tsk->pid == 1;
> > > > +	return task_pid_nr(tsk) == 1;
> > > >  }
> > > >  
> > > >  extern struct pid *cad_pid;
> > > > @@ -1930,7 +1930,7 @@ extern struct task_struct *idle_task(int cpu);
> > > >   */
> > > >  static inline bool is_idle_task(const struct task_struct *p)
> > > >  {
> > > > -	return p->pid == 0;
> > > > +	return task_pid(p) == &init_struct_pid;
> > > >  }
> > > >  extern struct task_struct *curr_task(int cpu);
> > > >  extern void set_curr_task(int cpu, struct task_struct *p);
> > > 
> > > Why would you ever want to do this? It just makes these tests more
> > > expensive for no gain what so ff'ing ever.
> > 
> > Backups are generally considered a good idea, but in this case, I'd
> > quote:
> > 	"A man with one watch knows what time it is. A man with two is
> > 	never certain."
> 
> Except that's not the case, with namespaces there's a clear hierarchy
> and the task_struct::pid is the one true value aka. root namespace.

Peter, I agonized over the access efficiency of dropping this one or the
duplicate in task_struct::pids and this one was far easier to drop in
terms of somehow always forcing
task->pids[PIDTYPE_PID].pid->numbers[0].nr to point to task->pid.

It should be possible to audit the kernel to make certain task->pid is
only ever written at the time of task creation and copied to its own
task->pids[PIDTYPE_PID].pid->numbers[0].nr at the time of task creation
so that the two values are consistent.  Continuously auditing the kernel
so this is the case would be a bit more of a challenge.

Would it be reasonable to suggest task_struct::pid only be accessed by
the existing inlined task_pid_nr() converted to const?

The goal is to gain assurance that any PIDs referred to in audit logs
are indisputable.

Would you be alright with doing away with task_struct::tgid?

> Furthermore idle threads really are special and it doesn't make sense to
> address them in any but the root namespace, doubly so because only
> kernel space does this.

If that is the case, and the above holds true, then we don't need the
second hunk, I agree.

> As for the init thread, that function is called is_global_init() for
> crying out loud, what numb nut doesn't get that that's supposed to be
> using the root namespace?

A process in another pid namespace?  If that's never going to happen,
then drop it.

> Seriously, you namespace guys should stop messing things up and
> confusing yourselves and others.

"you namespace guys"?  I'm not a namespace guy.  I'm a rusty kernel
network security guy taking on audit, trying to prepare it for moving
into a more and more namespace-enabled place of
containerization/light-virtualization.

We aren't ready for it yet, but there is demand to run additional auditd
daemons in other pid namespaces and some of this work is trying to move
in that direction.

This patchset certainly wasn't intended to be ready for adoption just
yet.  It was this sort of discussion I was hoping to have.

- RGB

--
Richard Guy Briggs <rbriggs@...hat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/