|
Message-ID: <20110715130409.GA2700@openwall.com> Date: Fri, 15 Jul 2011 17:04:09 +0400 From: Solar Designer <solar@...nwall.com> To: Vasiliy Kulikov <segoon@...nwall.com> Cc: NeilBrown <neilb@...e.de>, kernel-hardening@...ts.openwall.com, James Morris <jmorris@...ei.org>, Linus Torvalds <torvalds@...ux-foundation.org>, linux-kernel@...r.kernel.org, Greg Kroah-Hartman <gregkh@...e.de>, Andrew Morton <akpm@...ux-foundation.org>, "David S. Miller" <davem@...emloft.net>, Jiri Slaby <jslaby@...e.cz>, Alexander Viro <viro@...iv.linux.org.uk>, linux-fsdevel@...r.kernel.org, KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>, Eric Paris <eparis@...hat.com>, Stephen Smalley <sds@...ho.nsa.gov>, Willy Tarreau <w@....eu>, Sebastian Krahmer <krahmer@...e.de> Subject: Re: [PATCH] move RLIMIT_NPROC check from set_user() to do_execve_common() Neil, Vasiliy - On Fri, Jul 15, 2011 at 11:38:23AM +0400, Vasiliy Kulikov wrote: > On Fri, Jul 15, 2011 at 17:06 +1000, NeilBrown wrote: > > How about this then? > > AFAIU, with this patch: > > 1) setuid() doesn't fail in NPROC exceed case. > 2) NPROC is forced on execve() after setuid(). > 3) execve() fails only if NPROC was exceeded during setuid() execution. > 4) Other processes of the same user doesn't suffer from "occasional" > execve() failures. > > If it is correct, then I like the patch :) I'm not convinced this has any advantages over the patch Vasiliy had posted (which simply moved the RLIMIT_NPROC check), but I don't mind, with one important correction: Although in the primary use cases we're considering, setuid() is very soon followed by execve(), this doesn't always have to be the case. There may be cases where the process would sleep between these two calls (for a variety of reasons). It would be surprising to see a process fail on execve() because of RLIMIT_NPROC when that limit had been reached, say, days ago and is no longer reached at the time of execve(). Thus, if we go with Neil's proposal (adding/checking an extra flag), we should also re-check the process count against RLIMIT_NPROC on execve(), and fail the exec only when both the flag is set and the current process count is excessive (still or again). Also, if we go with a patch like this, it brings up the question of whether the flag should be preserved or reset on fork(). I think it should be preserved. > It does RLIMIT_NPROC > enforcement without mixing other execve() calls like -ow patch did. "Mixing other execve() calls" (4 on the list above) is not obviously undesirable. Thus, either Vasiliy's original patch or Neil's patch with the addition mentioned above would be fine by me. Thanks, Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.