prctl: set-mm -- Rework interface

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.

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.

Current PR_SET_MM code forbids to modify fields if no CAP_SYS_RESOURCE granted, but rather relies on one who use this interface is passing more-less sane values (though the values must pass the basic validation procedure).

It seems a better approach is to eliminate CAP_SYS_RESOURCE check but provide 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 members to be updated.

Most intensive work is done in validate_prctl_map_locked helper, because we need to make sure the values are valid. Thus we do

- check the values are laying inside available user address space
- stack, brk, command line arguments and evnironment variables must point to already existing VMA
- values must be ordered (start < end)
- if RLIMITs are defined don't allow to exceed it with new values

Since it uses prctl_set_mm_exe_file_locked helper, updating the exe-file link is one-shot action for security reason.

I believe the old interface should be deprecated and ripped off in a couple of kernel releases if noone 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.

Signed-off-by Cyrill Gorcunov <[email protected]>

Cc Kees Cook <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Michael Kerrisk <[email protected]>

---
 include/uapi/linux/prctl.h |   19 ++++
 kernel/sys.c               |  188 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 206 insertions(+), 1 deletion(-)

Index: linux-2.6.git/include/uapi/linux/prctl.h
===================================================================

include/uapi/linux/prctl.h +5 -0

+	unsigned long	end_code;
+	unsigned long	start_data;
+	unsigned long	end_data;
+	unsigned long	start_brk;
+	unsigned long	brk;

include/uapi/linux/prctl.h +10 -0

+
+	error |= __prctl_check_addr_space(prctl_map, start_code);
+	error |= __prctl_check_addr_space(prctl_map, end_code);
+	error |= __prctl_check_addr_space(prctl_map, start_data);
+	error |= __prctl_check_addr_space(prctl_map, end_data);
+	error |= __prctl_check_addr_space(prctl_map, start_stack);
+	error |= __prctl_check_addr_space(prctl_map, start_brk);
+	error |= __prctl_check_addr_space(prctl_map, brk);
+	error |= __prctl_check_addr_space(prctl_map, arg_start);
+	error |= __prctl_check_addr_space(prctl_map, arg_end);

include/uapi/linux/prctl.h +14 -0

+	if (!stack_vma) {
+		error = -EINVAL;
+		goto out;
+	}
+#define __prctl_check_vma(mm, addr) find_vma(mm, addr) ? 0 : -EINVAL
+	error |= __prctl_check_vma(mm, prctl_map->start_brk);
+	error |= __prctl_check_vma(mm, prctl_map->brk);
+	error |= __prctl_check_vma(mm, prctl_map->arg_start);
+	error |= __prctl_check_vma(mm, prctl_map->arg_end);
+	error |= __prctl_check_vma(mm, prctl_map->env_start);
+	error |= __prctl_check_vma(mm, prctl_map->env_end);
+	if (error)
+		goto out;
+#undef __prctl_check_vma

include/uapi/linux/prctl.h line 126

Quoted 3 lines

include/uapi/linux/prctl.h +3 -0

+
+struct prctl_mm_map {
+	unsigned long	start_code;

"unsigned long" has different sizes on x86_64 and x86, so a compat is required for x32 processes on x64 kernel.

kernel/sys.c exit: · line 1735

Quoted 5 lines

kernel/sys.c +5 -0

+
+	/*
+	 * Stack, brk, command line arguments and environment must exist.
+	 */
+	stack_vma = find_vma(mm, prctl_map->start_stack);

Why do we not use __prctl_check_vma here?

kernel/sys.c exit: · line 1742

Quoted 8 lines

kernel/sys.c +7 -0

+	if (!stack_vma) {
+		error = -EINVAL;
+		goto out;
+	}
+#define __prctl_check_vma(mm, addr) find_vma(mm, addr) ? 0 : -EINVAL
+	error |= __prctl_check_vma(mm, prctl_map->start_brk);
+	error |= __prctl_check_vma(mm, prctl_map->brk);

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.

Current PR_SET_MM code forbids to modify fields if no CAP_SYS_RESOURCE granted, but rather relies on one who use this interface is passing more-less sane values (though the values must pass the basic validation procedure).

It seems a better approach is to eliminate CAP_SYS_RESOURCE check but provide 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 members to be updated.

Most intensive work is done in validate_prctl_map_locked helper, because we need to make sure the values are valid. Thus we do

- check the values are laying inside available user address space
- stack, brk, command line arguments and evnironment variables must point to already existing VMA
- values must be ordered (start < end)
- if RLIMITs are defined don't allow to exceed it with new values

Since it uses prctl_set_mm_exe_file_locked helper, updating the exe-file link is one-shot action for security reason.

I believe the old interface should be deprecated and ripped off in a couple of kernel releases if noone 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.

Signed-off-by Cyrill Gorcunov <[email protected]>

Cc Kees Cook <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Michael Kerrisk <[email protected]>

Ping. Guys, any commens please?

Quoted 1 line · On Tue, 8 Jul 2014 23:08:49 +0400 Cyrill Gorcunov <[email protected]> wrote:

Ping. Guys, any commens please?

Well, allowing a process to modify pretty deep internals like this is always scary from a security point of view, and now we're removing CAP_SYS_RESOURCE protections. Yikes. Convince me we aren't handing out root here.

The changelog doesn't make it clear (to me) why this is actually being done. criu runs unprivileged? What's the requirement here?

struct prctl_mm_map could do with a nice comment explaining its role in the world.

I'm not seeing a coherent description of the proposed userspace interface. We'll eventually want to update the prctl manpage for this, so how about laying out all the needed details now, at patch review time so we can see what is proposed.

Why isn't the newly-added code under #ifdef CONFIG_CHECKPOINT_RESTORE?

Still the good news about all this members we modify -- they are used for statistics mostly except brk/stack related members but they are checked very carefully to not exceed the limits (if the limits are set).

Quoted 2 lines

struct prctl_mm_map could do with a nice comment explaining its role in the world.

ok, i'll update

Quoted 4 lines

I'm not seeing a coherent description of the proposed userspace interface. We'll eventually want to update the prctl manpage for this, so how about laying out all the needed details now, at patch review time so we can see what is proposed.

Sure, I'll write more descriptive comment since original "It takes a pointer of prctl_mm_map structure which carries all members to be updated" is too short.

Quoted 11 lines · Cyrill Gorcunov

Still the good news about all this members we modify -- they are used for statistics mostly except brk/stack related members but they are checked very carefully to not exceed the limits (if the limits are set).

Quoted 2 lines

struct prctl_mm_map could do with a nice comment explaining its role in the world.

ok, i'll update

Quoted 8 lines

Quoted 4 lines

I'm not seeing a coherent description of the proposed userspace interface. We'll eventually want to update the prctl manpage for this, so how about laying out all the needed details now, at patch review time so we can see what is proposed.

Sure, I'll write more descriptive comment since original "It takes a pointer of prctl_mm_map structure which carries all members to be updated" is too short.

Here is a way more descriptove changelog I hope. Please poke me if more details needed, or something should be improved/changed and etc.
---
From: Cyrill Gorcunov <[email protected]>
Subject: prctl: PR_SET_MM -- Introduce PR_SET_MM_MAP operation

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.

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.

Signed-off-by Cyrill Gorcunov <[email protected]>

Cc Kees Cook <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Michael Kerrisk <[email protected]>

---
 include/uapi/linux/prctl.h |   25 +++++
 kernel/sys.c               |  192 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 216 insertions(+), 1 deletion(-)

Index: linux-2.6.git/include/uapi/linux/prctl.h
===================================================================

include/uapi/linux/prctl.h +25 -0

@@ -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
===================================================================

kernel/sys.c +63 -0

@@ -1687,6 +1687,189 @@ 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(__map, __member)                              \
+       ({                                                                      \
+               int __rc;                                                       \
+               if ((unsigned long)__map->__member < mmap_max_addr &&           \
+                   (unsigned long)__map->__member >= mmap_min_addr)            \
+                       __rc = 0;                                               \
+               else                                                            \
+                       __rc = -EINVAL;                                         \
+               __rc;                                                           \
+       })
+
+       error |= __prctl_check_addr_space(prctl_map, start_code);
+       error |= __prctl_check_addr_space(prctl_map, end_code);
+       error |= __prctl_check_addr_space(prctl_map, start_data);
+       error |= __prctl_check_addr_space(prctl_map, end_data);
+       error |= __prctl_check_addr_space(prctl_map, start_stack);
+       error |= __prctl_check_addr_space(prctl_map, start_brk);
+       error |= __prctl_check_addr_space(prctl_map, brk);
+       error |= __prctl_check_addr_space(prctl_map, arg_start);
+       error |= __prctl_check_addr_space(prctl_map, arg_end);
+       error |= __prctl_check_addr_space(prctl_map, env_start);
+       error |= __prctl_check_addr_space(prctl_map, 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(mm, addr) find_vma(mm, (unsigned long)addr) ? 0 : -EINVAL
+       error |= __prctl_check_vma(mm, prctl_map->start_brk);
+       error |= __prctl_check_vma(mm, prctl_map->brk);
+       error |= __prctl_check_vma(mm, prctl_map->arg_start);
+       error |= __prctl_check_vma(mm, prctl_map->arg_end);
+       error |= __prctl_check_vma(mm, prctl_map->env_start);
+       error |= __prctl_check_vma(mm, prctl_map->env_end);
+       if (error)
+               goto out;
+#undef __prctl_check_vma
+
+       /*

kernel/sys.c exit: · line 1762

Quoted 12 lines

kernel/sys.c +12 -0

+
+       /*
+        * Make sure the pairs are ordered.
+        */
+#define __prctl_check_order(__map, __m1, __m2)                                 \
+       (unsigned long)__map->__m2 <= (unsigned long)__map->__m1
+       if (__prctl_check_order(prctl_map, start_code, end_code)        ||
+           __prctl_check_order(prctl_map, start_data, end_data)        ||
+           __prctl_check_order(prctl_map, arg_start, arg_end)          ||
+           __prctl_check_order(prctl_map, env_start, env_end))
+               goto out;
+#undef __prctl_check_order

This approach seems like a good solution given the security concerns with the earlier approach. I'm still pondering the implications, but as a minor style note, these macros are locally defined, but also all take at least a single identical argument in every usage. I would think it might be easier to read if they just used what they needed to directly.

#define __prctl_check_addr_space(__member)     \
      ((unsigned long)prctl_map->__member < mmap_max_addr &&           \
       (unsigned long)prctl_map->__member >= mmap_min_addr) ? 0 : -EINVAL

#define __prctl_check_vma(__member) \
           find_vma(mm, (unsigned long)prctl_map->__member) ? 0 : -EINVAL

Also, why change the symantics of the final macro? Seems like that one can use the same "error |=" style:

#define __prctl_check_order(__m1, __m2)         \
      prctl_map->__m1 < prctl_map->__m2 ? 0 : -EINVAL

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.

Quoted 5 lines · Cyrill Gorcunov

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 suppse this one should look better.
---
From: Cyrill Gorcunov <[email protected]>
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.

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 <[email protected]>

Cc Kees Cook <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Michael Kerrisk <[email protected]>

---
 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
===================================================================

include/uapi/linux/prctl.h +25 -0

@@ -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
===================================================================

kernel/sys.c +84 -0

@@ -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;

kernel/sys.c +35 -0

+
+       /*
+        * 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;
+

kernel/sys.c +5 -0

+
+       if (prctl_map.exe_fd != (u32)-1) {
+               error = prctl_set_mm_exe_file_locked(mm, prctl_map.exe_fd);
+               if (error)
+                       goto out;

kernel/sys.c +23 -1

+
+       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)))

Quoted 6 lines · Cyrill Gorcunov

Quoted 5 lines · Cyrill Gorcunov

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...

Quoted 4 lines

- @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()).

kernel/sys.c exit: · line 1783

Quoted 9 lines

kernel/sys.c +9 -0

+       /*
+        * 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.

Quoted 5 lines

kernel/sys.c +5 -0

+       rlim = rlimit(RLIMIT_STACK);
+       if (rlim < RLIM_INFINITY) {
+#ifdef CONFIG_STACK_GROWSUP
+               unsigned long left = stack_vma->vm_end - prctl_map->start_stack;
+#else

kernel/sys.c exit: · line 1794

Quoted 5 lines

kernel/sys.c +5 -0

+               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).

kernel/sys.c exit: · line 1839

Quoted 10 lines

kernel/sys.c +10 -0

+       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).

Quoted 11 lines

kernel/sys.c +11 -0

+
+       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;
+

kernel/sys.c exit: · line 1854

Quoted 4 lines

kernel/sys.c +4 -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.

Quoted 16 lines

kernel/sys.c +16 -0

+
+       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);

kernel/sys.c exit: · line 1873

Quoted 3 lines

kernel/sys.c +3 -0

+       return error;
+}
+#endif /* CONFIG_CHECKPOINT_RESTORE */

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).

Quoted 4 lines

- @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()).

Quoted 9 lines · Kees Cook

Quoted 4 lines

- @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()).

We don't want to reduce security. How can we get a process with a target exe link, which executes our code?

We can execute the target file and attach to it with ptrace. ptrace allows to inject and execute any code.

So if we are sure that we are able to do a previous scenario, we can safely change exe-link, can't we?

prctl already has a check of permissions to execute the target file. If we execute a file. What can prevent us to attach to the process with ptrace?

The file can have a suid bit, so after executing it we may lose ability to attach to it. To check that we can check that uid and gid is zero in a current userns (local root).

What else do we need to check?

Quoted 9 lines · Kees Cook

Quoted 4 lines

- @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()).

We don't want to reduce security. How can we get a process with a target exe link, which executes our code?

We can execute the target file and attach to it with ptrace. ptrace allows to inject and execute any code.

So if we are sure that we are able to do a previous scenario, we can safely change exe-link, can't we?

prctl already has a check of permissions to execute the target file. If we execute a file. What can prevent us to attach to the process with ptrace?

The file can have a suid bit, so after executing it we may lose ability to attach to it. To check that we can check that uid and gid is zero in a current userns (local root).

What else do we need to check?

Quoted 5 lines

The file can have a suid bit, so after executing it we may lose ability to attach to it. To check that we can check that uid and gid is zero in a current userns (local root).

Quoted 1 line

What else do we need to check?

Yeah, I think all the checks are sufficient, but I (and Julien) are still trying to think about side-effects.

It would be nice if these checks (like the rlimit checks) were merged into some common helper. That way if something changes in the exec path, it won't go missed in the c/r path.