Re: [PATCH 0/3] extend get/setrlimit to support setting rlimits external to a process (v7)

From: Neil Horman <nhorman@tuxdriver.com>
To: Ingo Molnar <mingo@elte.hu>
Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
	marcin.slusarz@gmail.com, tglx@linutronix.de, mingo@redhat.com,
	hpa@zytor.com, Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 0/3] extend get/setrlimit to support setting rlimits external to a process (v7)
Date: Mon, 2 Nov 2009 19:23:55 -0500	[thread overview]
Message-ID: <20091103002355.GB19891@localhost.localdomain> (raw)
In-Reply-To: <20091102185137.GA28803@elte.hu>

On Mon, Nov 02, 2009 at 07:51:37PM +0100, Ingo Molnar wrote:
> 
> * Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > > Have you ensured that no rlimit gets propagated during task init 
> > > into some other value - under the previously correct assumption that 
> > > rlimits dont change asynchronously under the feet of tasks?
> > 
> > I've looked, and the only place that I see the rlim array getting 
> > copied is via copy_signal when we're in the clone path.  The entire 
> > rlim array is copied from old task_struct to new task_struct under the 
> > protection of the current->group_leader task lock, which I also hold 
> > when updating via sys_setprlimit, so I think we're safe in this case.
> 
> I mean - do we set up any data structure based on a particular rlimit, 
> that can get out of sync with the rlimit being updated?
> 
> A prominent example would be the stack limit - we base address layout 
> decisions on it. Check arch/x86/mm/mmap.c. RLIM_INFINITY has a special 
> meaning plus we also set mmap_base() based on the rlim.
> 
Ah, I didn't consider those.  Yes it looks like some locking might be needed for
cases like that.  what would you suggest, simply grabbing the task lock before
looking at the rlim array?  That seems a bit heavy handed, especially if we want
to use the locking consistently.  What if we just converted the int array of
rlimit to atomic_t's?  Would that be sufficient, or still to heavy?

> Also, there appears to be almost no security checks in the new syscall! 
> We look up a PID but that's it - this code will allow unprivileged users 
> to lower various rlimits of system daemons - as if it were their own 
> limit. That's a rather big security hole.
> 
Yeah, I kept all the old checks in place, but didn't consider that other
processes might need additional security checks,   I guess the rule needs to be
that the callers uid needs to have CAP_SYS_RESOURCE and must match the uid of
the process being modified or be 0/root.  Is that about right?

Regards
Neil

> 	Ingo
>