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
| ||
|
Date: Sat, 23 Aug 2014 16:22:14 +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 01:53:02PM +0200, Oleg Nesterov wrote: > > > > It should protect from allocation/devetion/mergin of another vma. IOW when > > I lookup for vma I need to be sure it exist and won't disappear at least > > while I validate it. > > plus you need mmap_sem (at least for reading) when you update mm_struct, > this is clear. > > My question was why the whole function should be called under mmap_sem? > It could take it only around find_vma() + check(RLIMIT_STACK) ? Stricktly speaking yes, but don't forget we might need to update exe::file as well which requires lock to be taken. So it is simplier to take the read-lock for the whole function. > In fact I do not think we need this vma_stack/RLIMIT_STACK check at all. > It buys nithing and looks strange. RLIMIT_STACK is mostly for self-debugging, > to catch the, say, unlimited recursion. An application can trivially > create a stack region of arbitrary size. I'd seriously suggest to remove it. Look, allocate stack for self is not a problem (we do this for our parasite code which executes inside dumpee address space) but RLIMIT_STACK check is present in ipc shmem so I think we still need this check in a sake of consistency. (note this code doesn't require any special caps so I need to use as much checks/tests as possible). > > > > > + if (prctl_map.auxv_size) { > > > > + /* Last entry must be AT_NULL as specification requires */ > > > > + user_auxv[AT_VECTOR_SIZE - 2] = AT_NULL; > > > > + user_auxv[AT_VECTOR_SIZE - 1] = AT_NULL; > > > > + > > > > + task_lock(current); > > > > + memcpy(mm->saved_auxv, user_auxv, sizeof(user_auxv)); > > > > + task_unlock(current); > > > > > > Again, could you explain this task_lock() ? > > > > It is used for serialization access to saved_auxv, ie when we fill it > > with new data the other reader (via procfs interface) should wait until > > we finish. > > But proc_pid_auxv() doesn't take this lock? And even if it did, this lock > can't help. task_lock() is per-thread, and multiple threads (including > CLONE_VM tasks, vfork() for example) can share the same ->mm. > > This certainly doesn't look right. It takes this lock but indeed this won't help much. Looks like I need to use cred_guard_mutex instead of task_lock here, no? -- 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