Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756607AbaGVUHy (ORCPT ); Tue, 22 Jul 2014 16:07:54 -0400 Received: from mail-oa0-f41.google.com ([209.85.219.41]:41141 "EHLO mail-oa0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756225AbaGVUHw (ORCPT ); Tue, 22 Jul 2014 16:07:52 -0400 MIME-Version: 1.0 In-Reply-To: <20140711173625.GD19702@moon> References: <20140703143318.568554771@openvz.org> <20140703151102.842945837@openvz.org> <20140708190849.GC17860@moon.sw.swsoft.com> <20140708143830.ea078ef01e1d7d31276edbcd@linux-foundation.org> <20140708221336.GL17860@moon.sw.swsoft.com> <20140709141318.GM17860@moon.sw.swsoft.com> <20140709150604.GN17860@moon.sw.swsoft.com> <20140711173625.GD19702@moon> Date: Tue, 22 Jul 2014 13:07:51 -0700 X-Google-Sender-Auth: x9e0jnAwpSjF6UDPZqZtxeAju5w Message-ID: Subject: Re: [RFC 2/2] prctl: PR_SET_MM -- Introduce PR_SET_MM_MAP operation From: Kees Cook To: Cyrill Gorcunov Cc: Andrew Morton , LKML , Tejun Heo , Andrew Vagin , "Eric W. Biederman" , Serge Hallyn , Pavel Emelyanov , Vasiliy Kulikov , KAMEZAWA Hiroyuki , Michael Kerrisk , Julien Tinnes Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 11, 2014 at 10:36 AM, Cyrill Gorcunov wrote: > On Wed, Jul 09, 2014 at 07:06:04PM +0400, Cyrill Gorcunov wrote: >> >> Thanks a lot for comments, Kees! I tend to agre, leaving off the @prctl_map >> variable out of macros should make code also shorter, I'll update that's >> not the problem. Could you please re-check if I'm not missing something >> in security aspects when time permits. I asked Julien (now in CC) into look at this with me, and he had several comments that I've paraphrased/expanded on below... > > I suppse this one should look better. > --- > From: Cyrill Gorcunov > Subject: prctl: PR_SET_MM -- Introduce PR_SET_MM_MAP operation, v2 > > During development of c/r we've noticed that in case if we need to > support user namespaces we face a problem with capabilities in > prctl(PR_SET_MM, ...) call, in particular once new user namespace > is created capable(CAP_SYS_RESOURCE) no longer passes. > > A approach is to eliminate CAP_SYS_RESOURCE check but pass all > new values in one bundle, which would allow the kernel to make > more intensive test for sanity of values and same time allow us to > support checkpoint/restore of user namespaces. > > Thus a new command PR_SET_MM_MAP introduced. It takes a pointer of > prctl_mm_map structure which carries all the members to be updated. > > prctl(PR_SET_MM, PR_SET_MM_MAP, struct prctl_mm_map *, size) > > struct prctl_mm_map { > __u64 start_code; > __u64 end_code; > __u64 start_data; > __u64 end_data; > __u64 start_brk; > __u64 brk; > __u64 start_stack; > __u64 arg_start; > __u64 arg_end; > __u64 env_start; > __u64 env_end; > __u64 *auxv; > __u32 auxv_size; > __u32 exe_fd; > }; > > All members except @exe_fd correspond ones of struct mm_struct. > To figure out which available values these members may take here > are meanings of the members. > > - start_code, end_code: represent bounds of executable code area > - start_data, end_data: represent bounds of data area > - start_brk, brk: used to calculate bounds for brk() syscall > - start_stack: used when accounting space needed for command > line arguments, environment and shmat() syscall > - arg_start, arg_end, env_start, env_end: represent memory area > supplied for command line arguments and environment variables > - auxv, auxv_size: carries auxiliary vector, Elf format specifics > - exe_fd: file descriptor number for executable link (/proc/self/exe) > > Thus we apply the following requirements to the values > > 1) Any member except @auxv, @auxv_size, @exe_fd is rather an address > in user space thus it must be laying inside [mmap_min_addr, mmap_max_addr) > interval. > > 2) While @[start|end]_code and @[start|end]_data may point to an nonexisting > VMAs (say a program maps own new .text and .data segments during execution) > the rest of members should belong to VMA which must exist. > > 3) Addresses must be ordered, ie @start_ member must not be greater or > equal to appropriate @end_ member. > > 4) As in regular Elf loading procedure we require that @start_brk and > @brk be greater than @end_data. > > 5) If RLIMIT_DATA rlimit is set to non-infinity new values should not > exceed existing limit. Same applies to RLIMIT_STACK. > > 6) Auxiliary vector size must not exceed existing one (which is > predefined as AT_VECTOR_SIZE and depends on architecture). > > 7) File descriptor passed in @exe_file should be pointing > to executable file (because we use existing prctl_set_mm_exe_file_locked > helper it ensures that the file we are going to use as exe link has all > required permission granted). > > Now about where these members are involved inside kernel code: > > - @start_code and @end_code are used in /proc/$pid/[stat|statm] output; > > - @start_data and @end_data are used in /proc/$pid/[stat|statm] output, > also they are considered if there enough space for brk() syscall > result if RLIMIT_DATA is set; > > - @start_brk shown in /proc/$pid/stat output and accounted in brk() > syscall if RLIMIT_DATA is set; also this member is tested to > find a symbolic name of mmap event for perf system (we choose > if event is generated for "heap" area); one more aplication is > selinux -- we test if a process has PROCESS__EXECHEAP permission > if trying to make heap area being executable with mprotect() syscall; > > - @brk is a current value for brk() syscall which lays inside heap > area, it's shown in /proc/$pid/stat. When syscall brk() succesfully > provides new memory area to a user space upon brk() completion the > mm::brk is updated to carry new value; > > Both @start_brk and @brk are actively used in /proc/$pid/maps > and /proc/$pid/smaps output to find a symbolic name "heap" for > VMA being scanned; > > - @start_stack is printed out in /proc/$pid/stat and used to > find a symbolic name "stack" for task and threads in > /proc/$pid/maps and /proc/$pid/smaps output, and as the same > as with @start_brk -- perf system uses it for event naming. > Also kernel treat this member as a start address of where > to map vDSO pages and to check if there is enough space > for shmat() syscall; > > - @arg_start, @arg_end, @env_start and @env_end are printed out > in /proc/$pid/stat. Another access to the data these members > represent is to read /proc/$pid/environ or /proc/$pid/cmdline. > Any attempt to read these areas kernel tests with access_process_vm > helper so a user must have enough rights for this action; > > - @auxv and @auxv_size may be read from /proc/$pid/auxv. Strictly > speaking kernel doesn't care much about which exactly data is > sitting there because it is solely for userspace; > > - @exe_fd is referred from /proc/$pid/exe and when generating > coredump. We uses prctl_set_mm_exe_file_locked helper to update > this member, so exe-file link modification remains one-shot > action. Controlling exe_fd without privileges may turn out to be dangerous. At least things like tomoyo examine it for making policy decisions (see tomoyo_manager()). > > Still note that updating exe-file link now doesn't require sys-resource > capability anymore, after all there is no much profit in preventing setup > own file link (there are a number of ways to execute own code -- ptrace, > ld-preload, so that the only reliable way to find which exactly code > is executed is to inspect running program memory). > > I believe the old interface should be deprecated and ripped off > in a couple of kernel releases if no one against. > > To test if new interface is implemented in the kernel one > can pass PR_SET_MM_MAP_SIZE opcode and the kernel returns > the size of currently supported struct prctl_mm_map. > > v2: > - compact macros (by keescook@) > - wrap new code with CONFIG_ (by akpm@) > > Signed-off-by: Cyrill Gorcunov > Cc: Kees Cook > Cc: Tejun Heo > Cc: Andrew Morton > Cc: Andrew Vagin > Cc: Eric W. Biederman > Cc: H. Peter Anvin > Cc: Serge Hallyn > Cc: Pavel Emelyanov > Cc: Vasiliy Kulikov > Cc: KAMEZAWA Hiroyuki > Cc: Michael Kerrisk > --- > include/uapi/linux/prctl.h | 25 +++++ > kernel/sys.c | 194 ++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 218 insertions(+), 1 deletion(-) > > Index: linux-2.6.git/include/uapi/linux/prctl.h > =================================================================== > --- linux-2.6.git.orig/include/uapi/linux/prctl.h > +++ linux-2.6.git/include/uapi/linux/prctl.h > @@ -119,6 +119,31 @@ > # define PR_SET_MM_ENV_END 11 > # define PR_SET_MM_AUXV 12 > # define PR_SET_MM_EXE_FILE 13 > +# define PR_SET_MM_MAP 14 > +# define PR_SET_MM_MAP_SIZE 15 > + > +/* > + * This structure provides new memory descriptor > + * map which mostly modifies /proc/pid/stat[m] > + * output for a task. This mostly done in a > + * sake of checkpoint/restore functionality. > + */ > +struct prctl_mm_map { > + __u64 start_code; /* code section bounds */ > + __u64 end_code; > + __u64 start_data; /* data section bounds */ > + __u64 end_data; > + __u64 start_brk; /* heap for brk() syscall */ > + __u64 brk; > + __u64 start_stack; /* stack starts at */ > + __u64 arg_start; /* command line arguments bounds */ > + __u64 arg_end; > + __u64 env_start; /* environment variables bounds */ > + __u64 env_end; > + __u64 *auxv; /* auxiliary vector */ > + __u32 auxv_size; /* vector size */ > + __u32 exe_fd; /* /proc/$pid/exe link file */ > +}; > > /* > * Set specific pid that is allowed to ptrace the current task. > Index: linux-2.6.git/kernel/sys.c > =================================================================== > --- linux-2.6.git.orig/kernel/sys.c > +++ linux-2.6.git/kernel/sys.c > @@ -1687,6 +1687,191 @@ exit: > return err; > } > > +#ifdef CONFIG_CHECKPOINT_RESTORE > +/* > + * WARNING: we don't require any capability here so be very careful > + * in what is allowed for modification from userspace. > + */ > +static int validate_prctl_map_locked(struct prctl_mm_map *prctl_map) > +{ > + unsigned long mmap_max_addr = TASK_SIZE; > + struct mm_struct *mm = current->mm; > + struct vm_area_struct *stack_vma; > + unsigned long rlim; > + int error = 0; > + > + /* > + * Make sure the members are not somewhere outside > + * of allowed address space. > + */ > +#define __prctl_check_addr_space(__member) \ > + ({ \ > + int __rc; \ > + if ((unsigned long)prctl_map->__member < mmap_max_addr && \ > + (unsigned long)prctl_map->__member >= mmap_min_addr) \ > + __rc = 0; \ > + else \ > + __rc = -EINVAL; \ > + __rc; \ > + }) > + error |= __prctl_check_addr_space(start_code); > + error |= __prctl_check_addr_space(end_code); > + error |= __prctl_check_addr_space(start_data); > + error |= __prctl_check_addr_space(end_data); > + error |= __prctl_check_addr_space(start_stack); > + error |= __prctl_check_addr_space(start_brk); > + error |= __prctl_check_addr_space(brk); > + error |= __prctl_check_addr_space(arg_start); > + error |= __prctl_check_addr_space(arg_end); > + error |= __prctl_check_addr_space(env_start); > + error |= __prctl_check_addr_space(env_end); > + if (error) > + goto out; > +#undef __prctl_check_addr_space > + > + /* > + * Stack, brk, command line arguments and environment must exist. > + */ > + stack_vma = find_vma(mm, (unsigned long)prctl_map->start_stack); > + if (!stack_vma) { > + error = -EINVAL; > + goto out; > + } > +#define __prctl_check_vma(__member) \ > + find_vma(mm, (unsigned long)prctl_map->__member) ? 0 : -EINVAL > + error |= __prctl_check_vma(start_brk); > + error |= __prctl_check_vma(brk); > + error |= __prctl_check_vma(arg_start); > + error |= __prctl_check_vma(arg_end); > + error |= __prctl_check_vma(env_start); > + error |= __prctl_check_vma(env_end); > + if (error) > + goto out; > +#undef __prctl_check_vma > + > + /* > + * Make sure the pairs are ordered. > + */ > +#define __prctl_check_order(__m1, __m2) \ > + ((unsigned long)prctl_map->__m2 > \ > + (unsigned long)prctl_map->__m1) ? 0 : -EINVAL > + error |= __prctl_check_order(start_code, end_code); > + error |= __prctl_check_order(start_data, end_data); > + error |= __prctl_check_order(arg_start, arg_end); > + error |= __prctl_check_order(env_start, env_end); > + if (error) > + goto out; > +#undef __prctl_check_order > + > + error = -EINVAL; > + > + /* > + * @brk should be after @end_data in traditional maps. > + */ > + if (prctl_map->start_brk <= prctl_map->end_data || > + prctl_map->brk <= prctl_map->end_data) > + goto out; > + > + /* > + * Neither we should allow to override limits if they set. > + */ > + rlim = rlimit(RLIMIT_DATA); > + if (rlim < RLIM_INFINITY) { > + if ((prctl_map->brk - prctl_map->start_brk) + > + (prctl_map->end_data - prctl_map->start_data) > rlim) > + goto out; > + } I think this has an integer overflow in it. This could be avoided by checking brk vs start_brk with an additional __prctl_check_order call. This is done for start_data and end_data already. > + > + rlim = rlimit(RLIMIT_STACK); > + if (rlim < RLIM_INFINITY) { > +#ifdef CONFIG_STACK_GROWSUP > + unsigned long left = stack_vma->vm_end - prctl_map->start_stack; > +#else > + unsigned long left = prctl_map->start_stack - stack_vma->vm_start; > +#endif > + if (left > rlim) > + goto out; > + } There should be a __prctl_check_order for stack_start vs stack_vma->vm_end (and another in the stack growsdown case). > + > + /* > + * Someone is trying to cheat the auxv vector. > + */ > + if (prctl_map->auxv && prctl_map->auxv_size > sizeof(mm->saved_auxv)) > + goto out; > + error = 0; > +out: > + return error; > +} > + > +static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data_size) > +{ > + struct prctl_mm_map prctl_map = { .exe_fd = (u32)-1, }; > + unsigned long user_auxv[AT_VECTOR_SIZE]; > + struct mm_struct *mm = current->mm; > + int error = -EINVAL; > + > + BUILD_BUG_ON(sizeof(user_auxv) != sizeof(mm->saved_auxv)); > + > + if (opt == PR_SET_MM_MAP_SIZE) > + return put_user((unsigned int)sizeof(prctl_map), > + (unsigned int __user *)addr); > + > + if (data_size != sizeof(prctl_map)) > + return -EINVAL; > + > + if (copy_from_user(&prctl_map, addr, sizeof(prctl_map))) > + return -EFAULT; > + > + down_read(&mm->mmap_sem); > + > + if (validate_prctl_map_locked(&prctl_map)) > + goto out; > + > + if (prctl_map.auxv && prctl_map.auxv_size) { > + up_read(&mm->mmap_sem); > + memset(user_auxv, 0, sizeof(user_auxv)); > + error = copy_from_user(user_auxv, > + (const void __user *)prctl_map.auxv, > + prctl_map.auxv_size); > + down_read(&mm->mmap_sem); > + if (error) > + goto out; > + } "prctl_map.auxv" should be removed from this if condition (i.e. make sure any auxv_size does, in fact, attempt to write to the .auxv location). > + > + if (prctl_map.exe_fd != (u32)-1) { > + error = prctl_set_mm_exe_file_locked(mm, prctl_map.exe_fd); > + if (error) > + goto out; > + } > + > + if (prctl_map.auxv && prctl_map.auxv_size) { > + user_auxv[AT_VECTOR_SIZE - 2] = 0; > + user_auxv[AT_VECTOR_SIZE - 1] = 0; > + > + task_lock(current); > + memcpy(mm->saved_auxv, user_auxv, sizeof(user_auxv)); > + task_unlock(current); > + } This auxv if should probably be consolidated with the first one. And it may be worthwhile to mention this is making sure AT_NULL is at the end. > + > + mm->start_code = prctl_map.start_code; > + mm->end_code = prctl_map.end_code; > + mm->start_data = prctl_map.start_data; > + mm->end_data = prctl_map.end_data; > + mm->start_brk = prctl_map.start_brk; > + mm->brk = prctl_map.brk; > + mm->start_stack = prctl_map.start_stack; > + mm->arg_start = prctl_map.arg_start; > + mm->arg_end = prctl_map.arg_end; > + mm->env_start = prctl_map.env_start; > + mm->env_end = prctl_map.env_end; > + > + error = 0; > +out: > + up_read(&mm->mmap_sem); > + return error; > +} > +#endif /* CONFIG_CHECKPOINT_RESTORE */ > + > static int prctl_set_mm(int opt, unsigned long addr, > unsigned long arg4, unsigned long arg5) > { > @@ -1695,9 +1880,16 @@ static int prctl_set_mm(int opt, unsigne > struct vm_area_struct *vma; > int error; > > - if (arg5 || (arg4 && opt != PR_SET_MM_AUXV)) > + if (arg5 || (arg4 && (opt != PR_SET_MM_AUXV && > + opt != PR_SET_MM_MAP && > + opt != PR_SET_MM_MAP_SIZE))) > return -EINVAL; > > +#ifdef CONFIG_CHECKPOINT_RESTORE > + if (opt == PR_SET_MM_MAP || opt == PR_SET_MM_MAP_SIZE) > + return prctl_set_mm_map(opt, (const void __user *)addr, arg4); > +#endif > + > if (!capable(CAP_SYS_RESOURCE)) > return -EPERM; > To avoid future errors, the rlimit checks should probably go into some common place, so that the same functions are called during rlimit checks when "classic" modification of fields such as ->brk happen (for instance in sys_brk). Thanks! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/