Message-ID: <20140823201137.GK25918@moon>
Date:	Sun, 24 Aug 2014 00:11:37 +0400
From:	Cyrill Gorcunov <gorcunov@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Kees Cook <keescook@...omium.org>, Tejun Heo <tj@...nel.org>,
	Andrew Vagin <avagin@...nvz.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Vasiliy Kulikov <segoon@...nwall.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Julien Tinnes <jln@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: + prctl-pr_set_mm-introduce-pr_set_mm_map-operation-v3.patch
 added to -mm tree

On Sat, Aug 23, 2014 at 09:29:15PM +0200, Oleg Nesterov wrote:
> On 08/23, Cyrill Gorcunov wrote:
> >
> > On Sat, Aug 23, 2014 at 05:32:22PM +0200, Oleg Nesterov wrote:
> > >
> > > And btw, where do you see RLIMIT_STACK in do_shmat() ?
> >
> > Indirectly, though start_stack pointer. We assign it in setup_arg_pages taking into
> > account RLIMIT_STACK value, then do_shmat operates with start_stack pointer,
> > thus when we setup some new value into start_stack we at least should
> > not exceed old RLIMIT_STACK?
> 
> Still can't understand how do_shmat() connects to RLIMIT_STACK, even
> indirectly. setup_arg_pages() obviously uses RLIMIT_STACK, but with or
> without the patch I sent do_shmat() behaviour does not depend on
> RLIMIT_STACK at all.

setup_arg_pages sets up start_stack member and obeys RLIMIT_STACK limit
so if there were some rlimit set the start_stack will point to space
allocated with known size, then do_shmat takes start_stack and checks
if there space left. So initial value of start_stack is limited to
rlimit setting. When setup_arg_pages is called the start_stack
has rlimit constrain, so it looks to me pretty natural to do the
same in prctl, no?

> > > > arg_start, arg_end, env_start, env_end
> > > >        really point to existing VMAs, strictly speaking the probgram can unmap
> > > >        all own VMAs except executable one and continue running without problem
> > > >        but this is not that practical I think and at first iteration I prefer
> > > >        more severe tests here on VMAs
> > >
> > > But, again, for what? There are only used to report this info via /proc/.
> >
> > Because it's close to how loading elf proceeds. This prctl is like emulating
> > micro-elf loader ;) Indeed there is no problem for kernel if all these members
> > are pointing to some already umapped VMAs,
> 
> Yes. And whatever checks you add into prctl(), an application can simply
> do mmap() before prctl() just to fool it, and unmap (say) arg_start right
> after that.

yes it can, but prctl on itsown actually not much usefull until you need
to mimic "another program" procfs output like we do in criu :)

> > but earlier we required cap-sysadmin
> > to be grated so that if someone calls for this prctl he must be knowing what
> > he is doing, and if some other unpredicted change in kernel code cause this
> > prctl to panic the kernel such action could be initiated by sysadmin only,
> > not regular user. Now I've released the security requirement so I thought
> > let be more paranoid and require all VMAs to present, all rlimits to be
> > in good shape and such even if the members we're modifying are used for
> > procfs statistics output mostly, we always have a way to relax this tests
> > but not the reverse. That was the main idea ;)
> 
> Following this logic you should also verify that KSTK_ESP(group_leader)
> falls into ->start_stack area or return an error otherwise.

And probably I should, I'll re-check.

> 
> As for elf loader, of course it sets ->start_stack/etc correctly and they
> can't point to nowehere, but this is only because it actually creates these
> segments. And I guess the c/r tools should do this "correctly" too, but
> prctl() itself can never verify this.
> 
> OK, lets look at this from another angle. Suppose that an application switches
> to another stack and unmaps ->start_stack area. Now, how are you going to
> restore it correctly if ->start_stack points to nowhere? Are you going
> to "fix" ->start_stack? I do not think this would be correct. Or, are you
> going to create the additional vma just to make prctl() happy?

Actually all the programs we were c/r'ing are "normal" ones who doesn't
mangle own contents but I fear yes, with current approach if some vma's
are missing prctl refuse to proceed. Oleg, I'm cooking the patch which
addresses all your comments and relaxes the restrictions I simply need
once again grep all members to be sure it's safe. Hopefully I'll finish
tomorrow and show the patch, still if there some other concerns please
notify me I'm happy to address any of them!
--
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/

Powered by blists - more mailing lists