proc: protect /proc/<pid>/* files across execve

Procfs files and other important objects may contain sensitive information which must not be seen, inherited or processed across execve. The commit e268337dfe26dfc7efd422a804dbb27977a3cccc tries to fix the same problem, but that leaves all the other sensitive /proc/<pid>/* files which includes the {maps,smaps,numa_maps} files that can be used to bypass ASLR (there is a PoC to demonstrate it). We should also note that currently the /proc/<pid>/mem fix will pin mm_structs.

The following series tries to take another path to solve the same problem by using a global execve counter and each task will have its own exec ID, this way we can know if the processed files are attached to the reader or the target tasks. With this solution we do not pin mm_structs of dead processes and we can track special objects at each syscall.

There is a new proc_file_private struct which can be used to hold internal data related to opened /proc/<pid>/* files, we also use it to store the exec_id of the reader or the target tasks, it depends on the files being processed. This struct offers a consistent and unified way to protect all these sensitive files even the other /proc/* files.
There are also some new helper functions.

Currently we perform the protection in two different ways:

1) Use the target exec_id to bind files to their exec_id task:

For the REG files /proc/<pid>/{environ,pagemap,mem} we set the exec_id of the proc_file_private to the target task, and we continue with permission checks at open time, later on each read/write call the permission checks are done + check the target exec_id if it equals the exec_id of the proc_file_private that was set at open time, in other words we bind the file to its task's exec_id, this way new exec programs can not operate on the passed fd.

2) Use the reader exec_id to track reader behaviour (aggressive checks):

For the /proc/<pid>/{maps,smaps,numa_maps} we set the exec_id of the proc_file_private to the current (reader) exec_id, and the permission checks are only performed at read time + the exec_id check against reader, this way we are sure that we are still dealing with the same reader. We do this since currently it is not clear if the permission checks at open time will work with glibc FORTIFY_SOURCE protection and without permission checks at each syscall using the target exec_id will not work.

Hopefully it seems that perhaps we can work-around this, if the exec_id design is accepted then I will split /proc/<pid>/{maps,smaps,numa_maps} internal functions and implement proper permission checks so we can bind all these files to their task's exec_id (track target instead of reader).

For the ONE and INF files /proc/<pid>/{stack,syscall,io,auxv,...} we also use the reader exec_id since these sensitive files share their internal logic with the other less important files, and performing permission checks at open time will just break things.

Notes:
The exec_id idea was taken from the recent grsecurity patches, but I have made some design changes so if there are bugs they are mine.

This was also discussed in this kernel-hardening thread:
http://www.openwall.com/lists/kernel-hardening/2012/02/10/1

This thread also includes other procfs files problems which currently I am working on, I will try to continue with an other patch series as soon as possible. Thanks to Vasiliy Kulikov and Solar Designer for their comments.

Finally I will just add Alan's thread that also explain the problem with some historical discussions:
http://lkml.org/lkml/2012/1/29/35

Hope to get feedback to avoid new problems. Thanks.

Djalal Harouni (9):
exec: add a global execve counter
proc: add proc_file_private struct to store private information
proc: new proc_exec_id_ok() helper function
proc: protect /proc/<pid>/* INF files from reader across execve
proc: add protection support for /proc/<pid>/* ONE files
proc: protect /proc/<pid>/* ONE files from reader across execve
proc: protect /proc/<pid>/{maps,smaps,numa_maps}
proc: protect /proc/<pid>/{environ,pagemap} across execve
proc: improve and clean up /proc/<pid>/mem protection

 fs/exec.c             |   11 ++
 fs/proc/array.c       |    4 +
 fs/proc/base.c        |  303 ++++++++++++++++++++++++++++++++++++++++++-------
 fs/proc/internal.h    |   31 +++++-
 fs/proc/task_mmu.c    |  106 +++++++++++++++--
 include/linux/sched.h |   32 +++++
 6 files changed, 431 insertions(+), 56 deletions(-)

Procfs files and other important objects may contain sensitive information which must not be seen, inherited or processed across execve. The commit e268337dfe26dfc7efd422a804dbb27977a3cccc tries to fix the same problem.

Given that consideration this patch introduces two counters:
A global atomic execve counter that will be incremented on every do_execve_common() call, and an atomic exec_id member for the task_struct.

The task's exec_id can be used as a security check to see and to test if some special objects belong to the appropriate process image since it will be a uniq id on each execve operation.

There is also new helper functions:
get_task_exec_id() to get the task's exec_id and task_exec_id_ok() to check if the provided exec_id equals the task's exec_id.

The exec_id idea is taken from the recent grsecurity patches.

---
 fs/exec.c             |   11 +++++++++++
 include/linux/sched.h |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

index 153dee1..fb8ccae 100644
@@ -1448,6 +1448,14 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 EXPORT_SYMBOL(search_binary_handler);
 
 /*
+ * A global execve counter that we need to set atomically.
+ * It will be incremented on every do_execve_common() this way we can use
+ * it to check if some special objects belong to the appropriate process
+ * image.
+ */
+static atomic64_t exec_counter = ATOMIC_INIT(0);
+
+/*
  * sys_execve() executes a new program.
  */
 static int do_execve_common(const char *filename,

@@ -1460,6 +1468,7 @@ static int do_execve_common(const char *filename,
 	struct files_struct *displaced;
 	bool clear_in_exec;
 	int retval;
+	u64 exec_id;
 	const struct cred *cred = current_cred();
 
 	/*

@@ -1542,6 +1551,8 @@ static int do_execve_common(const char *filename,
 		goto out;
 
 	/* execve succeeded */
+	exec_id = atomic64_inc_return(&exec_counter);
+	atomic64_set(&current->exec_id, exec_id);
 	current->fs->in_exec = 0;
 	current->in_execve = 0;
 	acct_update_integrals(current);

index 0657368..762f206 100644
@@ -1420,6 +1420,9 @@ struct task_struct {
 #endif
 	seccomp_t seccomp;
 
+/* Execve counter: will be used to check if objects belong to the appropriate
+ * process image */
+	atomic64_t exec_id;
 /* Thread group tracking */
    	u32 parent_exec_id;
    	u32 self_exec_id;

@@ -1752,6 +1755,35 @@ static inline int is_global_init(struct task_struct *tsk)
 	return tsk->pid == 1;
 }
 
+/**
+ * task_get_exec_id - get the task's exec_id
+ * @task: Task struct to operate on.
+ *
+ * Get the task's exec_id.
+ */
+static inline u64 get_task_exec_id(struct task_struct *task)
+{
+	return atomic64_read(&task->exec_id);
+}
+
+/**
+ * task_exec_id_ok - check if the task's exec_id equals the provided
+ * exec_id.
+ * @task: Task struct to check against.
+ * @exec_id: Execve ID.
+ *
+ * Check if the task's exec_id equals the provided exec_id. Use it to
+ * protect special objects.
+ *
+ * It will be more effective if the check is delayed as mush as possible
+ * to avoid any new execve surprises especially if we are checking against
+ * target task's exec_id.
+ */
+static inline int task_exec_id_ok(struct task_struct *task, u64 exec_id)
+{
+	return atomic64_read(&task->exec_id) == exec_id;
+}
+
 /*
  * is_container_init:
  * check whether in the task is init in its own pid namespace.

1.7.1

This seems horribly expensive on most 32-bit architectures, including very much x86-32. That atomic64_inc_return() is not cheap. It's possible that it's basically an impossible operation to do atomically on certain platforms, causing it to use some random spinlock instead.

I wonder if we couldn't make something much cheaper, since we don't actually care about it being globally incrementing, we just care about it being globally unique. IOW, it could easily be a 56-bit per-cpu counter along with the CPU number in the high bits or something like that. Avoiding the whole atomicity issue, and thus avoiding the main reason those things are really expensive.

IOW, something like

   cpu = get_cpu();
   .. increment percpu 64-bit counter ..
   id = counter * MAX_CPUS + cpu;
   put_cpu(cpu);

or equivalent would seem to be a potentially much cheaper approach.

Linus

Actually, we don't even need it to be "globally unique" afaik. It really needs to be "relatively unique", since it's always compared within the confines of a particular task.

So the problem with using the existing "self_exec_id" is that it's trivially the same across tasks. But a *tuple* of <struct task_struct, thread-specific exec_id> doesn't have that issue, so as far as I can tell we could easily make *that* the unique ID.

I wonder if the number part of exec_id would even have to be 64-bit. I think I can do about 10000 execves per second if I make the program a small static one - and that's on a fast CPU. And it's a per-thread counter, so you can't scale it with lots of CPU's. So it would take something like four days to wrap. Hmm..

Linus

Actually, using a pure counter is horrible, because even if it takes four days to wrap, it *will* wrap, and the attacker can just count his own execve's.

If, instead, you were to use a counter that counts *independently* of execve's, you're much better off.

And if you use one that is free - because the CPU implements it natively - you're even better off.

IOW, why is the exec-id just the time stamp counter (on any random cpu
- we really don't care)? That should be safe even in just 32 bits exactly because it's not under the control of the user.

And it's zero cost for us to update.

Linus

Four days (for a 32-bit counter) is just not enough, so the counter needs to be e.g. 64-bit as proposed. A 64-bit counter won't wrap during lifetime of a system.

The CPUs' timestamp counters were not designed for security. I would not be too surprised if some implementation of a CPU architecture (maybe emulated, maybe under a hypervisor) has such timestamp counter granularity that we may see the same value across a second execve().

Also, we'd need extra code for archs/CPUs that lack timestamp counters.

I'd rather not rely on the timestamp counters across CPUs being in sync, and if they are not in sync then we may see the same value again (on one CPU vs. another). So at least we'd need to record the CPU number as well.

32 bits is just not enough even if not under control of an attacker. If there's some low chance of hitting the same counter value on execve() vs. procfs file access and there's no lockout on multiple counter mismatches, an attacker may simply try that in a loop, and with 32-bit values might have a good chance of succeeding in a reasonable amount of time (such as a few days).

Alexander

A 64-bit counter is OK on 32-bit platforms as well as long as it's not *atomic*.

Linus's scheme of using the CPU ID for the high bits would solve that particular problem IMO. Each CPU would have its own count set apart in a percpu area, accessible via __this_cpu_inc() or so.

16 bits for the CPU ID and 48 bits for the actual count should be enough for everyone! ;-) It wraps in about 700 years, with current CPU speeds and assuming that exec() will be relatively slow in the future as well.

Some other kernel code might make use of such a fast, global generation count as well, so if this is reasonably abstracted out and named properly it would not just be a single-purpose security facility. DEFINE_GENCOUNT() or so? [the count itself would not be reused, of course.]

Not using the TSC would certainly make this logic simpler and faster - which is a big plus for any security measure.

Thanks,

Ingo

I don't think an exec id trick works that well here. It needs to bind to the actual *object* being used and refcount it in the cases that matter, then have a proper way of ensuring we clean up references such as a list of objects to zap hooked to each task struct

So something like

	struct foo_node {
		struct list_node node;
		struct proc_object *ref;
	};
		
	ref = NULL;
	if (foo->ptr != NULL
		ref = kref_get(foo->ptr);

And in the task exit paths walk the node list doing a kref_put/NULL

Add a suitable lock and it ought to be able to generically do that for anything you need to clobber.

You've still got a sort of race however, just as the proposed execid base code. You can pass the fd and access the proc function *as* the exec occurs. Assuming your ref counting is valid and you use new objects after the exec that ought to just mean you get the data for the old mm struct, which seems fine to me. It's logically equivalent to having asked a microsecond before the exec rather than during it.

Alan

Here I assume that you are talking about the target task.

Yes but proc inode which are created on the fly can also be released by the reader before the target exits, so do we want to walk the node list each time release is called by the reader ?

The current implementation tries to track target, but as noted in the other thread we can just track the reader and in this case we do not need atomic for task_struct nor for the proc_file_private, a simple u64 comparison will do the job

But perhaps what you propose is better, I'll try to think more about it.

IMO the proposed patch do not suffer from this race if we do the propre permission checks just after setting the exec_id at open, or do permission checks and then check exec_id at read. And here I'm talking about target tracking. If we just check reader (current) then I assume that there are no room for races.

Thanks.

tixxdz
http://opendz.org

Yes it will use spinlocks, and before that we also hold the current->signal->cred_guard_mutex during all the do_execve_common(), not to mention the search_binary_handler() logic which will test binary format... it can even call request_module()

There is the first:
exec_id = atomic64_inc_return(&exec_counter); /* increment global */

and the second one:
atomic64_set(&current->exec_id, exec_id); /* set task exec_id */

I've added this since when we track the target task we do not want to race against it when it does its execve call or when we set the exec_id at open time of the /proc/<pid>/* files.

I can remove this second one but in this situation we must track only reader (current), which is an aggressive behaviour, actually it may be safer to do so. I've made the target track change to fit the desired behaviour of "bind files to their process image".

And when we are at it, we should only allow reading of /proc/<pid>/mem, at least it's only info leak in case there are future changes that can affect the current protections.

tixxdz
http://opendz.org

I think these are _relatively_ cheap considering that it's execve(). If we can do e.g. 10 million of atomic64_inc_return()'s per second (and I think we can do a lot more, although I did not benchmark this), but only 10000 of execve()'s per second, then the performance impact is 0.1%. I admit that 0.1% is significant, but I used worst-case guesstimates here; the actual number might be more like 0.01% (50 million vs. 5 thousand) or even 0.002% (200 million vs. 4 thousand). (200 million is 15 CPU cycles at 3 GHz, which may be a reasonable optimistic estimate. 4 thousand is a realistic execve() speed for dynamically-linked programs.)

This makes sense to me. We just need to ensure that the per-CPU counter is still large enough that it won't wrap. 56 bits is enough (considering that the attack has to run on a single CPU of the target system, unlike e.g. an attack on a cipher with a 56-bit key could be), but there's little room for reducing this further (such as to support many more than 256 CPUs in a system). So if we use this approach, maybe we should simply keep a 64-bit per-CPU counter and a 32-bit CPU number, for a 96-bit id. This may even be faster on execve() (no need to combine the two values into one).

I don't know which one of these approaches has lower overhead on current systems. My _guess_ is that atomic64_inc_return() may well be faster in many cases.

Alexander

(change subject)

In fact I think it would be nice to kill parent_exec_id/self_exec_id.

Afaics, this only problem is clone(CLONE_PARENT | SIGXXX). I expect the answer is "no, can break existing applications", but I'll ask anyway.

Can't we change this? IOW, can't we modify copy_process

	- p->exit_signal = (clone_flags & CLONE_THREAD) ? -1 : (clone_flags & CSIGNAL);
	+ p->exit_signal =
	+	(clone_flags & CLONE_THREAD) ? -1 :
	+	(clobe_flags & CLONE_PARENT) ? current->group_leader->exit_signal :
	+	(clone_flags & CSIGNAL);

(or simply use SIGCHLD instead of group_leader->exit_signal).

Then we can kill parent_exec_id/self_exec_id if me modify de_thread() to set ->exit_signal = SIGCHLD for every child.

I am also asking because the change above looks like the fix to me. The child must not control its ->exit_signal, it is the parent who decides which signal the child should use for notification.

And to me, clone(CLONE_PARENT | SIGXXX) looks like a violation of rule above.

Oleg.

SIGXXX is for doing things like AIO with threads, but it would never be used together with CLONE_PARENT, that would be odd and wrong.

So I think we could disallow that - or at least try. See if anybody notices, and if it breaks anything.

The rule about the Linux ABI is not that the ABI is set in stone. It's that we can't break any existing binaries. And *maybe* there are users of CLONE_PARENT and special signals, but it sounds unlikely and would probably confuse real programs. So feel free to just try it (early in the 3.4 merge window - not at this point, though).

Linus

UserModeLinux is using CLONE_PARENT | CLONE_FILES | SIGCHLD. Is this a problem?

Thanks,
//richard

This depends on how the forking process was created, and what its parent expects.

If it was forked with SIGCHLD as well, then no problems. Otherwise the parent can notice the difference.

Oleg.

OK, nobody seems to object.

Andrew, could you take this patch?

As for self_exec_id/parent_exec_id, this needs cleanups and fixes in any case. But perhaps we can kill them after this change.

Oleg.

The child must not control its ->exit_signal, it is the parent who decides which signal the child should use for notification.

This means that CLONE_PARENT should not use "clone_flags & CSIGNAL", the forking task is the sibling of the new process and their parent doesn't control exit_signal in this case.

This patch uses ->exit_signal of the forking process, but perhaps we should simply use SIGCHLD.

We read group_leader->exit_signal lockless, this can race with the ORIGINAL_SIGNAL -> SIGCHLD transition, but this is fine.

Potentially this change allows to kill self_exec_id/parent_exec_id.

---
 kernel/fork.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

index b77fd55..34d7ed1 100644
@@ -1310,7 +1310,13 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	clear_all_latency_tracing(p);
 
 	/* ok, now we should be set up.. */
-	p->exit_signal = (clone_flags & CLONE_THREAD) ? -1 : (clone_flags & CSIGNAL);
+	if (clone_flags & CLONE_THREAD)
+		p->exit_signal = -1;
+	else if (clone_flags & CLONE_PARENT)
+		p->exit_signal = current->group_leader->exit_signal;
+	else
+		p->exit_signal = (clone_flags & CSIGNAL);
+
 	p->pdeath_signal = 0;
 	p->exit_state = 0;
 

1.5.5.1

So why is it "current->group_leader->exit_signal" rather than the much more logical (imho) and simpler "current->exit_signal"?

Linus

This would be wrong if current is not the main thread. In this case current->exit_signal = -1. Only group_leader has the "real" exit_signal used for notification.

Historically "exit_signal = -1" meant different things, currently it only means "I am not the leader", see thread_group_leader().

I'll write another email tomorrow. Yes, I do remember I promised the security fixes in this area, sorry for delay.

Oleg.

Andrew, these changes are orthogonal to "CLONE_PARENT shouldn't allow to set ->exit_signal" in -mm.

Please review/comment. The patches are really simple I hope, but with or without them I do not understand eligible_child().

	/* Wait for all children (clone and not) if __WALL is set;
	 * otherwise, wait for clone children *only* if __WCLONE is
	 * set; otherwise, wait for non-clone children *only*.  (Note:
	 * A "clone" child here is one that reports to its parent
	 * using a signal other than SIGCHLD.) */
	if (((p->exit_signal != SIGCHLD) ^ !!(wo->wo_flags & __WCLONE))
	    && !(wo->wo_flags & __WALL))
		return 0;

OK, but exec/exit can change ->exit_signal "in between".

For example. The parent clones the !SIGCHLD child and does do_wait(__WCLONE). The child execs. The parent still sleeps until child exits but do_wait() returns ECHILD. And more, this also depends on who does exec in mt case.

Or the parent execs. In this case eligible_child() depends on when the child exits, before or after exec.

Anyway, the current check in exit_notify() is wrong, 2/2 tries to fix this. But both patches add the (hopefully minor) user-visible changes wrt eligible_child().

Really, I think de_thread() should set ->exit_signal = SIGCHLD for each child. This looks simple and understandable, and with the CLONE_PARENT change above we can kill self_exec_id/parent_exec_id.

May be de_thread() should also do __wake_up_parent() if it changes ->exit_signal. Or perhaps we should change eligible_child() to check p->cloned_with_not_SIGCHLD. I dunno. I hope this doesn't matter and nobody cares.

OTOH, I never understood why we should reset ->exit_signal if the child execs but the parent is still the same.

3/3 is offtopic minor cleanup. Imho setmax_mm_hiwater_rss() in de_thread() looks strange.

Oleg.

exit_notify() checks "tsk->self_exec_id != tsk->parent_exec_id" to handle the "we have changed execution domain" case.

We can change do_thread() to always set ->exit_signal = SIGCHLD and remove this check to simplify the code.

We could change setup_new_exec() instead, this looks more logical because it increments ->self_exec_id. But note that de_thread() already resets ->exit_signal if it changes the leader, let's keep both changes close to each other.

Note that we change ->exit_signal lockless, this changes the rules. Thereafter ->exit_signal is not stable under tasklist but this is fine, the only possible change is OLDSIG -> SIGCHLD. This can race with eligible_child() but the race is harmless. We can race with reparent_leader() which changes our ->exit_signal in parallel, but it does the same change to SIGCHLD.

The noticeable user-visible change is that the execing task is not "visible" to do_wait()->eligible_child(__WCLONE) right after exec. To me this looks more logical, and this is consistent with mt case.

---
 fs/exec.c     |    3 +++
 kernel/exit.c |    7 +------
 2 files changed, 4 insertions(+), 6 deletions(-)

index 92ce83a..218d074 100644
@@ -975,6 +975,9 @@ static int de_thread(struct task_struct *tsk)
 	sig->notify_count = 0;
 
 no_thread_group:
+	/* we have changed execution domain */
+	tsk->exit_signal = SIGCHLD;
+
 	if (current->mm)
 		setmax_mm_hiwater_rss(&sig->maxrss, current->mm);
 

index 7b36288..522c1aa 100644
@@ -827,14 +827,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	 * If the parent exec id doesn't match the exec id we saved
 	 * when we started then we know the parent has changed security
 	 * domain.
-	 *
-	 * If our self_exec id doesn't match our parent_exec_id then
-	 * we have changed execution domain as these two values started
-	 * the same after a fork.
 	 */
 	if (thread_group_leader(tsk) && tsk->exit_signal != SIGCHLD &&
-	    (tsk->parent_exec_id != tsk->real_parent->self_exec_id ||
-	     tsk->self_exec_id != tsk->parent_exec_id))
+	    tsk->parent_exec_id != tsk->real_parent->self_exec_id)
 		tsk->exit_signal = SIGCHLD;
 
 	if (unlikely(tsk->ptrace)) {

1.5.5.1

exit_notify() changes ->exit_signal if the parent already did exec. This doesn't really work, we are not going to send the signal now if there is another live thread or the exiting task is traced. The parent can exec before the last dies or the tracer detaches.

Move this check into do_notify_parent() which actually sends the signal.

The user-visible change is that we do not change ->exit_signal, and thus the exiting task is still "clone children" for do_wait()->eligible_child(__WCLONE). Hopefully this is fine, the current logic is racy anyway.

---
 kernel/exit.c   |   14 --------------
 kernel/signal.c |    9 +++++++++
 2 files changed, 9 insertions(+), 14 deletions(-)

index 522c1aa..bd9f4c9 100644
@@ -818,20 +818,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	if (group_dead)
 		kill_orphaned_pgrp(tsk->group_leader, NULL);
 
-	/* Let father know we died
-	 *
-	 * Thread signals are configurable, but you aren't going to use
-	 * that to send signals to arbitrary processes.
-	 * That stops right now.
-	 *
-	 * If the parent exec id doesn't match the exec id we saved
-	 * when we started then we know the parent has changed security
-	 * domain.
-	 */
-	if (thread_group_leader(tsk) && tsk->exit_signal != SIGCHLD &&
-	    tsk->parent_exec_id != tsk->real_parent->self_exec_id)
-		tsk->exit_signal = SIGCHLD;
-
 	if (unlikely(tsk->ptrace)) {
 		int sig = thread_group_leader(tsk) &&
 				thread_group_empty(tsk) &&

index c73c428..b09cf3b 100644
@@ -1642,6 +1642,15 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
 	BUG_ON(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 
+	if (sig != SIGCHLD) {
+		/*
+		 * This is only possible if parent == real_parent.
+		 * Check if it has changed security domain.
+		 */
+		if (tsk->parent_exec_id != tsk->parent->self_exec_id)
+			sig = SIGCHLD;
+	}
+
 	info.si_signo = sig;
 	info.si_errno = 0;
 	/*

1.5.5.1

Minor cleanup. de_thread()->setmax_mm_hiwater_rss() looks a bit strange, move it into exec_mmap() which plays with old_mm.

---
 fs/exec.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

index 218d074..7f84e86 100644
@@ -848,6 +848,7 @@ static int exec_mmap(struct mm_struct *mm)
 	if (old_mm) {
 		up_read(&old_mm->mmap_sem);
 		BUG_ON(active_mm != old_mm);
+		setmax_mm_hiwater_rss(&tsk->signal->maxrss, old_mm);
 		mm_update_next_owner(old_mm);
 		mmput(old_mm);
 		return 0;

@@ -978,9 +979,6 @@ no_thread_group:
 	/* we have changed execution domain */
 	tsk->exit_signal = SIGCHLD;
 
-	if (current->mm)
-		setmax_mm_hiwater_rss(&sig->maxrss, current->mm);
-
 	exit_itimers(sig);
 	flush_itimer_signals();
 

1.5.5.1

I do wonder if we shouldn't just consider the "struct mm_struct" pointer to *be* the unique exec ID. It's what /proc/pid/mem does, and it works fine, and allows us to just use a normal pointer as the unique ID.

Just increment the mm_count for the thing, and hold a reference to it, and now you're all done.

By definition an execve() will change the mm struct, and if you have a refcount to the old one, it won't be re-used. And it's not a huge allocation, although it would definitely be good to put that thing on a diet.

And as long as we use 'mm_count', not 'mm_users', it won't pin anything else in memory. That's part of the whole point of the doubly-refcounted thing.

Linus

Please Linus have you checked the:
[PATCH 9/9] proc: improve and clean up /proc/<pid>/mem protection

That keeping the mm struct wont work, since it will eat memory and the OOM-killer will kill some innocent processes, and the abuse can only be catched by the VFS.

What's your opinion on it ?

And if we do the same for all the /proc/<pid>/* files and even if you clear the VM of the old process, some other information will still be available, even if it is not useful, this is not consistent.

Is it ok if the code keeps different objects related to dead processes alive ?

tixxdz
http://opendz.org

That's the point. I made the mistake of using mm_users initially, but ysing mm_count - which is what I said to use (and what Oleg fixed things to in commit 6d08f2c71397) should *not* have that problem. It just keeps the 'struct mm_struct' itself around.

What's the advantage? You replace it with *another* allocation, and a 64-bit thing that is much less useful.

The size of the patch also speaks for itself:

 fs/proc/base.c |   99 ++++++++++++++++++++++++++++++++++++++++++++------------

and it's more complex and uses more memory on average (the refcount thing is *free* for usual cases).

I do agree that it would be nicer if mm_struct was a bit smaller, but at the same time, I really don't see the advantage of replacing it with another allocation entirely that makes the code just more complicated.

Linus

And that mm_struct will explode and only the VFS will catch it.

Given 1024 processes * (RLIMIT_NOFILE 1024 - 3) == ~1020000

more than 1020000 mm structs (all of dead processes ?)

A quick test on a default ubuntu:
cat /proc/sys/fs/file-max
388411

So we are able to keep around 388411 dead mm_struct in memory, just try it.

Our embedded devices will suffer, serial login will be killed, getty, ... ssh root owned ... I've experienced this.

I'll try to post more numbers.

That allocation is much smaller and it can be used to protect all /proc/<pid>/* files in a unified way. Consistency seems the right thing to do, even the /proc/<pid>/{syscall,stack,...} that do not operate on mm structs will be protected. I don't know how you will protect these files ?

We can even remove the allocation and just reference the exec_id or use other fields of the file struct.

I'm must admit that grsecurity just did the check in the seq files, they did not add anything, this can leave some other files unprotected, but it is cheaper.

Well, it protect as from wraps.

or have an agreement.

That's not fair since most of the logic of /proc/<pid>/* files is in fs/proc/base.c and we can clean the code, there are some duplicated functions in my patch, which we can improve.

As I've said we can improve it.

I'll take all the feedback I got and re-work the patches, in the meantime some of these sensitive files need to be protected especially the /proc/<pid>/{maps,smaps,numa_maps}

I'll re-submit.

Thanks for the feedback.

tixxdz
http://opendz.org

Umm.

I think your argument is totally braindead and wrong.

My counter-argument is very simple: "So what?"

Those mm_structs are small. They are something like a couple of hundred bytes. If you really worry about open files, you should worry about the size of the inode, and people using the "pipe()" system call. Then you have those open files with an inode, *and* several kB of data that can be trivially filled by the user with a simple "write()" that they never need read.

So "struct mm_struct" is totally irrelevant, and not in any way a special thing. It's not the biggest, it's not the most interesting, and it's simply not interesting. You're barking up the wrong tree.

None of it has anything to do with 'struct mm_struct', though, has it?

I suspect the real thing to do is to just make the OOM killer look at how many files are open too. Make each open file count as 4kB (or more), and use it when deciding what to kill. Fix the actual real problem instead of trying to fix one small detail - and one that isn't even the right small detail.

Linus

I thought that keeping these dead bytes alive is not the best solution, this is why we have all these counters...

My first solution to protect these files was to emulate what you are currently suggesting, your patch + Olge's patch. I've written a similar patch to protect /proc/<pid>/{maps,smaps} but later after more thoughts, an atomic operation (which I'll re-work) and a comparison (without allocations) seems more benefical if we achieve the same protection.

I must say that I'm confused here:
Sometimes we try to align structs and later we just ignore this ?

/sys/kernel/slab/mm_struct/object_size on 64bit gives:
1232

Ok, yes I see.

To clarify: I was worrying about the downsides of keeping dead data alive.

Yes it's not the biggest but it's related to the problem, any way here we are not trying to fix mm_struct, yes we are protecting procfs files.

Perhaps another simple/clean patch will do the job, otherwise protecting these files is the priority. Thank you for the reminder.

These files need protections and do not operate on mm:
/proc/<pid>/{syscall,stack}

And we need to add the open file operation to most of the /proc/<pid>/* sensitive files.

With 64 processes or less OOM killer can start killing innocent processes.

(and what about legitimate open files ?)

Thanks for the comments.

tixxdz
http://opendz.org

Some /proc/<pid>/{syscall,stack,...} do not operate on mm_struct so why we should add the: "acquire a reference to mm, get exec_id and mmput".

For the signal_struct currently I don't know, from a comment it seems that signal_struct can be shared! I don't know.

Yes someone who knows this should check it.

Thanks.

tixxdz
http://opendz.org

This could be simpler, just read the counter under task_lock(). And unless I misread the next patches syscall/stack can use current->mm lockless.

OK, nevermind.

Yes, it is shared, and that is why it makes sense for the per-process data. All threads in the thread group (process) have the same ->signal. And unlike ->mm, ->signal survives after exec.

Oleg.

In order to store and to track the private information specific to each procfs opened file we convert the 'proc_maps_private' struct which is used by the /proc/$pid/{maps,smaps,numa_maps} files to 'proc_file_private' which can now be used by all the /proc/<pid>/* and even other /proc/* files to store private information that must be kept across syscalls.

The proc_file_private struct includes an exec_id member which can be used to track and to protect special and sensitive procfs files. In general it will be set at open time to the task's exec_id to bind the file to this task.

The proc_file_private struct offers the way to make the checks and to bind special procfs files to the appropriate task in a consistent and unified way.

---
 fs/proc/internal.h |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

index 2925775..a8968f6 100644
@@ -61,10 +61,19 @@ extern const struct file_operations proc_pagemap_operations;
 extern const struct file_operations proc_net_operations;
 extern const struct inode_operations proc_net_inode_operations;
 
-struct proc_maps_private {
+/*
+ * Internal proc_file_private is used to track procfs files being
+ * processed especially the ones that varies during runtime.
+ */
+struct proc_file_private {
+	/* The Execve ID of the task, use this to protect special procfs
+	 * files. Must be set at open time. */
+	u64 exec_id;
 	struct pid *pid;
+	struct inode *inode;
 	struct task_struct *task;
 #ifdef CONFIG_MMU
+	/* For /proc/pid/{maps,smaps...} */
 	struct vm_area_struct *tail_vma;
 #endif
 };

1.7.1

proc_exec_id_ok() will be used for /proc/<pid>/* files checks to see if the task's exec_id equals the exec_id of the proc_file_private struct.

---
 fs/proc/internal.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

index a8968f6..bed27a8 100644
@@ -10,6 +10,7 @@
  */
 
 #include <linux/proc_fs.h>
+#include <linux/sched.h>
 
 extern struct proc_dir_entry proc_root;
 #ifdef CONFIG_PROC_SYSCTL

@@ -95,6 +96,25 @@ static inline int proc_fd(struct inode *inode)
 	return PROC_I(inode)->fd;
 }
 
+/**
+ * proc_exec_id_ok - check if the task's exec_id equals the exec_id of
+ * the proc_file_private.
+ * @task: Task struct to check against.
+ * @proc_private: The proc_file_private struct.
+ *
+ * Check if the exec_id of the two structs are equal. Use it to protect
+ * special procfs files when the fd is passed to a new execve (i.e. suid)
+ *
+ * It will be more effective if the check is delayed as mush as possible
+ * to avoid any new execve surprises especially if we are checking against
+ * target task's exec_id.
+ */
+static inline int proc_exec_id_ok(struct task_struct *task,
+				  struct proc_file_private *proc_priv)
+{
+	return task_exec_id_ok(task, proc_priv->exec_id);
+}
+
 struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *ino,
 		struct dentry *dentry);
 int proc_readdir_de(struct proc_dir_entry *de, struct file *filp, void *dirent,

1.7.1

Protect all the INF files /proc/$pid/{auxv,limits,syscall,cmdline,io,...} from reader across execve. A process can open its /proc/self/* INF files and exec a setuid or a privileged program, then this last one will use the passed fd and leak its own sensitive information.

Since there are multiple INF files this patch provides a solution without any internal processing logic change.
The exec_id of the proc_file_private will be set to the reader's exec_id instead of the target's exec_id. Using the target's exec_id should be the natural choice since this will bind these files to their task, but in order to do that we must do permission checks (ptrace) at each syscall, and currently these checks are done at read time and only for some important procfs files. By using the current's (reader) exec_id we are sure that the reader process at read time did not perform an execve.

All the /proc/<pid>/* INF files are protected by a one hit check just before returning results to userspace.

This is an aggressive check compared to the target's exec_id check.

---
 fs/proc/base.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 47 insertions(+), 2 deletions(-)

index d4548dd..387e637 100644
@@ -610,15 +610,42 @@ static const struct inode_operations proc_def_inode_operations = {
 
 #define PROC_BLOCK_SIZE	(3*1024)		/* 4K page size but our output routines use some slack for overruns */
 
+static int proc_info_open(struct inode *inode, struct file *filp)
+{
+	struct proc_file_private *priv;
+	int ret = -ENOMEM;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ret;
+
+	/*
+	 * Use the exec_id of current to protect INF files. We do this
+	 * since there are no permission checks (ptrace...) at open, they
+	 * are done at read time, and only for the sensitive files.
+	 * By using the current's exec_id we are sure that the reader process
+	 * did not change.
+	 */
+	priv->exec_id = get_task_exec_id(current);
+	filp->private_data = priv;
+
+	return 0;
+}
+
 static ssize_t proc_info_read(struct file * file, char __user * buf,
 			  size_t count, loff_t *ppos)
 {
 	struct inode * inode = file->f_path.dentry->d_inode;
+	struct proc_file_private *priv = file->private_data;
 	unsigned long page;
-	ssize_t length;
-	struct task_struct *task = get_proc_task(inode);
+	ssize_t length = 0;
+	struct task_struct *task;
+
+	if (!priv)
+		return length;
 
 	length = -ESRCH;
+	task = get_proc_task(inode);
 	if (!task)
 		goto out_no_task;
 

@@ -631,8 +658,16 @@ static ssize_t proc_info_read(struct file * file, char __user * buf,
 
 	length = PROC_I(inode)->op.proc_read(task, (char*)page);
 
+	/* Check delayed */
+	if (!proc_exec_id_ok(current, priv)) {
+		length = 0;
+		goto out_free;
+	}
+
 	if (length >= 0)
 		length = simple_read_from_buffer(buf, count, ppos, (char *)page, length);
+
+out_free:
 	free_page(page);
 out:
 	put_task_struct(task);

@@ -640,9 +675,19 @@ out_no_task:
 	return length;
 }
 
+static int proc_info_release(struct inode *inode, struct file *filp)
+{
+	struct proc_file_private *priv = filp->private_data;
+
+	kfree(priv);
+	return 0;
+}
+
 static const struct file_operations proc_info_file_operations = {
+	.open		= proc_info_open,
 	.read		= proc_info_read,
 	.llseek		= generic_file_llseek,
+	.release	= proc_info_release,
 };
 
 static int proc_single_show(struct seq_file *m, void *v)

1.7.1

Sensitive information of the /proc/<pid>/* ONE files can be leaked if a process opens its own /proc/self/* files and exec a setuid or a privileged program, this last one will read from the fd and leak its own sensitive data.

Since there are multiple ONE files this patch will use the following
logic: the exec_id of the proc_file_private will be set to the reader's exec_id instead of the target's exec_id. Using the target's exec_id should be the natural choice since this will bind these files to their task, but in order to do that we must do permission checks (ptrace) at each syscall, and currently these checks are done at read time and only for some important procfs files. By using the current's (reader) exec_id we are sure that the reader process at read time did not perform an execve.

Currently this is the list of the ONE files that will use this protection:
/proc/<pid>/{personality,stat,stack} and each file will have its own check. This will be provided by the next patch.

This is an aggressive check compared to the target's exec_id check.

---
 fs/proc/base.c |   36 ++++++++++++++++++++++++++++++++----
 1 files changed, 32 insertions(+), 4 deletions(-)

index 387e637..6df8ddd 100644
@@ -692,12 +692,17 @@ static const struct file_operations proc_info_file_operations = {
 
 static int proc_single_show(struct seq_file *m, void *v)
 {
-	struct inode *inode = m->private;
+	struct proc_file_private *priv = m->private;
+	struct inode *inode;
 	struct pid_namespace *ns;
 	struct pid *pid;
 	struct task_struct *task;
-	int ret;
+	int ret = 0;
 
+	if (!priv)
+		return ret;
+
+	inode = priv->inode;
 	ns = inode->i_sb->s_fs_info;
 	pid = proc_pid(inode);
 	task = get_pid_task(pid, PIDTYPE_PID);

@@ -712,14 +717,37 @@ static int proc_single_show(struct seq_file *m, void *v)
 
 static int proc_single_open(struct inode *inode, struct file *filp)
 {
-	return single_open(filp, proc_single_show, inode);
+	struct proc_file_private *priv;
+	int ret = -ENOMEM;
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (priv) {
+		/* Protect ONE files with current's exec_id */
+		priv->exec_id = get_task_exec_id(current);
+		ret = single_open(filp, proc_single_show, priv);
+		if (!ret)
+			priv->inode = inode;
+		else
+			kfree(priv);
+	}
+	return ret;
+}
+
+static int proc_single_release(struct inode *inode, struct file *filp)
+{
+	struct seq_file *seq = filp->private_data;
+	int ret = 0;
+	if (seq) {
+		kfree(seq->private);
+		ret = single_release(inode, filp);
+	}
+	return ret;
 }
 
 static const struct file_operations proc_single_file_operations = {
 	.open		= proc_single_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= single_release,
+	.release	= proc_single_release,
 };
 
 static int mem_open(struct inode* inode, struct file* file)

1.7.1

Protect the /proc/<pid>/{stack,stat,personality} ONE files from reader across execve by checking the exec_id of the reader.

The check is against current's exec_id since there are no permission checks (ptrace) at open time and currently we do not want to change the internal logic of these files, so we just make sure that the reader process did not perform an execve at read time.

---
 fs/proc/array.c |    4 ++++
 fs/proc/base.c  |   34 +++++++++++++++++++++++-----------
 2 files changed, 27 insertions(+), 11 deletions(-)

index c602b8d..20b6f5e 100644
@@ -381,6 +381,10 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
 	permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
+
+	if (!proc_exec_id_ok(current, m->private))
+		return 0;
+
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);

index 6df8ddd..0a5383e 100644
@@ -305,6 +305,7 @@ static void unlock_trace(struct task_struct *task)
 static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 			  struct pid *pid, struct task_struct *task)
 {
+	struct proc_file_private *priv = m->private;
 	struct stack_trace trace;
 	unsigned long *entries;
 	int err;

@@ -320,17 +321,24 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 	trace.skip		= 0;
 
 	err = lock_trace(task);
-	if (!err) {
-		save_stack_trace_tsk(task, &trace);
+	if (err)
+		goto out_free;
 
-		for (i = 0; i < trace.nr_entries; i++) {
-			seq_printf(m, "[<%pK>] %pS\n",
-				   (void *)entries[i], (void *)entries[i]);
-		}
-		unlock_trace(task);
+	err = 0;
+	if (!proc_exec_id_ok(current, priv))
+		goto out_unlock;
+
+	save_stack_trace_tsk(task, &trace);
+
+	for (i = 0; i < trace.nr_entries; i++) {
+		seq_printf(m, "[<%pK>] %pS\n",
+			   (void *)entries[i], (void *)entries[i]);
 	}
-	kfree(entries);
 
+out_unlock:
+	unlock_trace(task);
+out_free:
+	kfree(entries);
 	return err;
 }
 #endif

@@ -3021,10 +3029,14 @@ static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 				struct pid *pid, struct task_struct *task)
 {
 	int err = lock_trace(task);
-	if (!err) {
+	if (err)
+		return err;
+
+	err = 0;
+	if (proc_exec_id_ok(current, m->private))
 		seq_printf(m, "%08x\n", task->personality);
-		unlock_trace(task);
-	}
+
+	unlock_trace(task);
 	return err;
 }
 

1.7.1

Protect the /proc/<pid>/{maps,smaps,numa_maps} files from reader across execve by checking its exec_id.

The best solution should be to bind these files to their task by using the target's exec_id and to perform permission checks at each syscall, but currently this is not possible, it will break glibc FORTIFY_SOURCE protection. For the moment the exec_id check is against the reader to be sure that the reader process is not playing tricks and did not perform an execve at read time.

Use the new 'proc_file_private' struct to protect these sensitive files, this struct can store and handle all the /proc/<pid>/{maps,smaps,numa_maps} internal data.

The proc_exec_id_ok() check is performed inside the functions that are responsible of constructing and reporting results.

---
 fs/proc/task_mmu.c |   37 ++++++++++++++++++++++++++-----------
 1 files changed, 26 insertions(+), 11 deletions(-)

index 7dcd2a2..96a0e4a 100644
@@ -90,7 +90,7 @@ static void pad_len_spaces(struct seq_file *m, int len)
 	seq_printf(m, "%*c", len, ' ');
 }
 
-static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma)
+static void vma_stop(struct proc_file_private *priv, struct vm_area_struct *vma)
 {
 	if (vma && vma != priv->tail_vma) {
 		struct mm_struct *mm = vma->vm_mm;

@@ -101,7 +101,7 @@ static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma)
 
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
-	struct proc_maps_private *priv = m->private;
+	struct proc_file_private *priv = m->private;
 	unsigned long last_addr = m->version;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma, *tail_vma = NULL;

@@ -168,7 +168,7 @@ out:
 
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	struct proc_maps_private *priv = m->private;
+	struct proc_file_private *priv = m->private;
 	struct vm_area_struct *vma = v;
 	struct vm_area_struct *tail_vma = priv->tail_vma;
 

@@ -181,7 +181,7 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void m_stop(struct seq_file *m, void *v)
 {
-	struct proc_maps_private *priv = m->private;
+	struct proc_file_private *priv = m->private;
 	struct vm_area_struct *vma = v;
 
 	if (!IS_ERR(vma))

@@ -193,15 +193,20 @@ static void m_stop(struct seq_file *m, void *v)
 static int do_maps_open(struct inode *inode, struct file *file,
 			const struct seq_operations *ops)
 {
-	struct proc_maps_private *priv;
+	struct proc_file_private *priv;
 	int ret = -ENOMEM;
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv) {
-		priv->pid = proc_pid(inode);
+		/*
+		 * Use current's exec_id since currently there are no
+		 * permission checks at open.
+		 */
+		priv->exec_id = get_task_exec_id(current);
 		ret = seq_open(file, ops);
 		if (!ret) {
 			struct seq_file *m = file->private_data;
 			m->private = priv;
+			priv->pid = proc_pid(inode);
 		} else {
 			kfree(priv);
 		}

@@ -278,9 +283,12 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 static int show_map(struct seq_file *m, void *v)
 {
 	struct vm_area_struct *vma = v;
-	struct proc_maps_private *priv = m->private;
+	struct proc_file_private *priv = m->private;
 	struct task_struct *task = priv->task;
 
+	if (!proc_exec_id_ok(current, priv))
+		return 0;
+
 	show_map_vma(m, vma);
 
 	if (m->count < m->size)  /* vma is copied successfully */

@@ -424,7 +432,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 
 static int show_smap(struct seq_file *m, void *v)
 {
-	struct proc_maps_private *priv = m->private;
+	struct proc_file_private *priv = m->private;
 	struct task_struct *task = priv->task;
 	struct vm_area_struct *vma = v;
 	struct mem_size_stats mss;

@@ -434,6 +442,9 @@ static int show_smap(struct seq_file *m, void *v)
 		.private = &mss,
 	};
 
+	if (!proc_exec_id_ok(current, priv))
+		return 0;
+
 	memset(&mss, 0, sizeof mss);
 	mss.vma = vma;
 	/* mmap_sem is held in m_start */

@@ -878,7 +889,7 @@ struct numa_maps {
 };
 
 struct numa_maps_private {
-	struct proc_maps_private proc_maps;
+	struct proc_file_private proc_maps;
 	struct numa_maps md;
 };
 

@@ -1005,7 +1016,7 @@ static int gather_hugetbl_stats(pte_t *pte, unsigned long hmask,
 static int show_numa_map(struct seq_file *m, void *v)
 {
 	struct numa_maps_private *numa_priv = m->private;
-	struct proc_maps_private *proc_priv = &numa_priv->proc_maps;
+	struct proc_file_private *proc_priv = &numa_priv->proc_maps;
 	struct vm_area_struct *vma = v;
 	struct numa_maps *md = &numa_priv->md;
 	struct file *file = vma->vm_file;

@@ -1018,6 +1029,9 @@ static int show_numa_map(struct seq_file *m, void *v)
 	if (!mm)
 		return 0;
 
+	if (!proc_exec_id_ok(current, proc_priv))
+		return 0;
+
 	/* Ensure we start with an empty set of numa_maps statistics. */
 	memset(md, 0, sizeof(*md));
 

@@ -1097,11 +1111,12 @@ static int numa_maps_open(struct inode *inode, struct file *file)
 	int ret = -ENOMEM;
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv) {
-		priv->proc_maps.pid = proc_pid(inode);
+		priv->proc_maps.exec_id = get_task_exec_id(current);
 		ret = seq_open(file, &proc_pid_numa_maps_op);
 		if (!ret) {
 			struct seq_file *m = file->private_data;
 			m->private = priv;
+			priv->proc_maps.pid = proc_pid(inode);
 		} else {
 			kfree(priv);
 		}

1.7.1

The /proc/<pid>/{environ,pagemap} are sensitive files which must be protected across execve to avoid information leaks.

These files are protected by attaching them to their task at open time by saving the exec_id of the target task, this way in read we just compare the target task's exec_id and the previously saved exec_id of the proc_file_private struct, in other words we just bind these files to their appropriate process image at open time. We do this since we are able to do proper permission checks (ptrace) at each syscall, so we do not care about the reader.

Another important rule is to set the exec_id of the target task before the permission checks at open, this way we do not race against target task execve, and it will be more effective if the exec_id check at read/write times are delayed as much as possible to be sure that the target task do not change during execve.

This patch adds the open file_operation to the /proc/<pid>/{environ,pagemap} so we are able to set the exec_id of the target task and to do the appropriate permission checks. The exec_id check is done in the related read file_operation.

---
 fs/proc/base.c     |   85 +++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/proc/task_mmu.c |   69 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 148 insertions(+), 6 deletions(-)

index 0a5383e..536cd65 100644
@@ -882,15 +882,66 @@ static const struct file_operations proc_mem_operations = {
 	.release	= mem_release,
 };
 
+static int environ_open(struct inode *inode, struct file *filp)
+{
+	struct proc_file_private *priv;
+	struct mm_struct *mm;
+	struct task_struct *task;
+	int ret = -ENOMEM;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ret;
+
+	ret = -ESRCH;
+	task = get_proc_task(filp->f_path.dentry->d_inode);
+	if (!task)
+		goto out_free;
+
+	/*
+	 * Use target task's exec_id to bind the file to the task.
+	 * Setup exec_id first then check for permissions.
+	 */
+	priv->exec_id = get_task_exec_id(task);
+	mm = mm_for_maps(task);
+	put_task_struct(task);
+
+	if (!mm) {
+		ret = -ENOENT;
+		goto out_free;
+	}
+
+	if (IS_ERR(mm)) {
+		ret = PTR_ERR(mm);
+		goto out_free;
+	}
+
+	filp->private_data = priv;
+	/* do not pin mm */
+	mmput(mm);
+
+	return 0;
+
+out_free:
+	kfree(priv);
+	return ret;
+}
+
 static ssize_t environ_read(struct file *file, char __user *buf,
 			size_t count, loff_t *ppos)
 {
-	struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
+	struct proc_file_private *priv = file->private_data;
+	struct task_struct *task;
 	char *page;
 	unsigned long src = *ppos;
-	int ret = -ESRCH;
+	int ret = 0;
 	struct mm_struct *mm;
 
+	if (!priv)
+		return ret;
+
+	ret = -ESRCH;
+	task = get_proc_task(file->f_dentry->d_inode);
 	if (!task)
 		goto out_no_task;
 

@@ -901,9 +952,15 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 
 
 	mm = mm_for_maps(task);
-	ret = PTR_ERR(mm);
-	if (!mm || IS_ERR(mm))
+	if (!mm) {
+		ret = -ENOENT;
+		goto out_free;
+	}
+
+	if (IS_ERR(mm)) {
+		ret = PTR_ERR(mm);
 		goto out_free;
+	}
 
 	ret = 0;
 	while (count > 0) {

@@ -925,6 +982,16 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 			break;
 		}
 
+		/*
+		 * Delay the check since access_process_vm() operates
+		 * on a task.
+		 */
+		if (!proc_exec_id_ok(task, priv)) {
+			/* There was an execv */
+			ret = 0;
+			break;
+		}
+
 		if (copy_to_user(buf, page, retval)) {
 			ret = -EFAULT;
 			break;

@@ -946,9 +1013,19 @@ out_no_task:
 	return ret;
 }
 
+static int environ_release(struct inode *inode, struct file *filp)
+{
+	struct proc_file_private *priv = filp->private_data;
+
+	kfree(priv);
+	return 0;
+}
+
 static const struct file_operations proc_environ_operations = {
+	.open		= environ_open,
 	.read		= environ_read,
 	.llseek		= generic_file_llseek,
+	.release	= environ_release,
 };
 
 static ssize_t oom_adjust_read(struct file *file, char __user *buf,

1.7.1

->open is duplicated.

Right, I'll unify the code in a generic open function that does:
* alloc and setup proc_file_private (which includes the exec_id)
* ptrace check using mm_for_maps()
unify only those who check PTRACE_MODE_READ
* save priv_file_proc.

This applies also to the release functions, I'll re-submit it.

Thanks Alexey.

tixxdz
http://opendz.org

The /proc/<pid>/mem is a sensitive file which needs proper protection.

Commit e268337dfe26dfc7efd422a804dbb27977a3cccc changes the handling of the /proc/<pid>/mem file by attaching and tracking the VM of the target process at open time which means that even after an execve the old VM will still be referenced.

Commit 6d08f2c7139790c268820a2e590795cb8333181a makes sure that the VM of the target process is not pinned which will prevent rlimits bypass but the internal kernel mm_struct will be.

The mm_struct is a big struct and this can have negative effects on the machine especially low memory ones if a process keep /proc/<pid>/mem open, fork and re-exec where the /proc/<pid>/mem is related to a dead or a zombie process.

Some tests regarding the current implementation:

1) slabtop results with 32 processes each one with 1000 opened file:

OBJS   ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
 26272  26270  99%    1.56K   1634       20     52288K mm_struct
 26685  26681  99%    0.69K   1251       23     20016K filp

...
108 103 95% 5.20K 18 6 576K task_struct

The number of mm_structs do not reflect the number of task_structs, and all these mm_structs are related to zombie/dead processes. In this case with 32 processes on low memory the OOM killer will be triggered killing other processes.

2) With enough memory and with a higher number of processes:

OBJS   ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
 34233  31414  91%    1.25K   2858       12     45728K mm_struct
 34944  32062  91%    0.38K   1664       21     13312K filp
 44835  41973  93%    0.19K   2135       21      8540K kmalloc-192

...
294 163 55% 4.88K 49 6 1568K task_struct

In this (2) situation we reach the VFS file-max value /proc/sys/fs/file-max which was 34869 on the tested system.

But It is not the job of the VFS to block this behaviour, and systems with higher file-max values will suffer.

Based on the above considerations and if we can achieve the same /proc/<pid>/mem protection then we should go for it.

Use the proc_file_private and its exec_id to track /proc/<pid>/mem usage. Set the exec_id to the target task's exec_id at open time, and later compare it on each read and write calls before processing the VM of the target which means we bind the /proc/<pid>/mem to its exec_id process at open time. Permission checks are also done on each syscall so we do not care about the reader, only the target is tracked and its mm_struct is not pinned, this means that on each syscall readers/writers will operate on the right mm_struct.

We must perform permission checks at each syscall.

The appropriate exec_id of the proc_file_private must be set at the right time just before doing the permission checks at open time, this way we do not race against target task execve. The exec_id checks at read and write time must also be done at the right time after permission checks when the mm is already grabbed.

---
 fs/proc/base.c |   99 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 78 insertions(+), 21 deletions(-)

index 536cd65..a9cc73c 100644
@@ -760,50 +760,99 @@ static const struct file_operations proc_single_file_operations = {
 
 static int mem_open(struct inode* inode, struct file* file)
 {
-	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
+	struct proc_file_private *priv;
+	struct task_struct *task;
 	struct mm_struct *mm;
+	int ret = -ENOMEM;
 
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ret;
+
+	ret = -ESRCH;
+	task = get_proc_task(file->f_path.dentry->d_inode);
 	if (!task)
-		return -ESRCH;
+		goto out_free;
 
+	/*
+	 * Use target task's exec_id to bind the current opened
+	 * /proc/<pid>/mem file to its process image.
+	 *
+	 * Setup exec_id first then check for permissions.
+	 */
+	priv->exec_id = get_task_exec_id(task);
 	mm = mm_access(task, PTRACE_MODE_ATTACH);
 	put_task_struct(task);
 
-	if (IS_ERR(mm))
-		return PTR_ERR(mm);
+	if (!mm) {
+		ret = -ENOENT;
+		goto out_free;
+	}
 
-	if (mm) {
-		/* ensure this mm_struct can't be freed */
-		atomic_inc(&mm->mm_count);
-		/* but do not pin its memory */
-		mmput(mm);
+	if (IS_ERR(mm)) {
+		ret = PTR_ERR(mm);
+		goto out_free;
 	}
 
 	/* OK to pass negative loff_t, we can catch out-of-range */
 	file->f_mode |= FMODE_UNSIGNED_OFFSET;
-	file->private_data = mm;
+	file->private_data = priv;
+
+	mmput(mm);
 
 	return 0;
+
+out_free:
+	kfree(priv);
+	return ret;
 }
 
 static ssize_t mem_rw(struct file *file, char __user *buf,
 			size_t count, loff_t *ppos, int write)
 {
-	struct mm_struct *mm = file->private_data;
+	struct proc_file_private *priv = file->private_data;
+	struct task_struct *task;
 	unsigned long addr = *ppos;
-	ssize_t copied;
+	ssize_t copied = 0;
 	char *page;
+	struct mm_struct *mm;
 
-	if (!mm)
-		return 0;
+	if (!priv)
+		return copied;
+
+	copied = -ESRCH;
+	task = get_proc_task(file->f_dentry->d_inode);
+	if (!task)
+		goto out_no_task;
 
+	copied = -ENOMEM;
 	page = (char *)__get_free_page(GFP_TEMPORARY);
 	if (!page)
-		return -ENOMEM;
+		goto out;
+
+	mm = mm_access(task, PTRACE_MODE_ATTACH);
+	if (!mm) {
+		copied = -ENOENT;
+		goto out_free;
+	}
+
+	if (IS_ERR(mm)) {
+		copied = PTR_ERR(mm);
+		goto out_free;
+	}
 
 	copied = 0;
-	if (!atomic_inc_not_zero(&mm->mm_users))
-		goto free;
+	/*
+	 * See if the processed /proc/<pid>/mem file belongs to the
+	 * original opened /proc/<pid>/ process image and that there was
+	 * no new execve.
+	 *
+	 * Do the check here since access_remote_vm() will operate
+	 * on the already grabbed mm.
+	 */
+	if (!proc_exec_id_ok(task, priv))
+		/* There was an execv */
+		goto out_mm;
 
 	while (count > 0) {
 		int this_len = min_t(int, count, PAGE_SIZE);

@@ -813,6 +862,10 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
 			break;
 		}
 
+		/*
+		 * Do not change this: we must operate on the previously
+		 * grabbed mm.
+		 */
 		this_len = access_remote_vm(mm, addr, page, this_len, write);
 		if (!this_len) {
 			if (!copied)

@@ -832,9 +885,13 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
 	}
 	*ppos = addr;
 
+out_mm:
 	mmput(mm);
-free:
+out_free:
 	free_page((unsigned long) page);
+out:
+	put_task_struct(task);
+out_no_task:
 	return copied;
 }
 

@@ -868,9 +925,9 @@ loff_t mem_lseek(struct file *file, loff_t offset, int orig)
 
 static int mem_release(struct inode *inode, struct file *file)
 {
-	struct mm_struct *mm = file->private_data;
-	if (mm)
-		mmdrop(mm);
+	struct proc_file_private *priv = file->private_data;
+
+	kfree(priv);
 	return 0;
 }
 

1.7.1

So the exec_id approach was totally broken when it was used for /proc/<pid>/mem, is there any reason to believe it's a good idea now?

It's entirely predictable, and you can make the exec_id match by simply forking elsewhere and then passing the fd around using unix domain sockets, since the exec_id is just updated by incrementing a counter.

I would in general suggest strongly against using exec_id for anything that involves files. It isn't designed for that, it's designed for the whole "check the parent exec_id" thing for ptrace, where that whole "pass things around to another process" approach doesn't work.

Linus

Yes the previously one was broken since it was not a global uniq exec_id, it was designed for threads tracking.

The current one is a global exec_id with uniq IDs, incremented on each do_execve_common() call.

For the fork one yes exec_id will match but we have the permission checks (ptrace) at each syscall, so even if two processes share the same exec_id the ptrace check should fail.

Yes it's predictable, but I don't see how you could pass the fd to another extern privileged process without failing at the exec_id check.

tixxdz
http://opendz.org

Yes exec_id for files can seem strange, but here we are speaking about virtual files that are related to the image of the process being run, these files are in reality just some portion of the process memory.

Linus please check the patch:
[PATCH 9/9] proc: improve and clean up /proc/<pid>/mem protection

I have explained why the current protection is not the best solution. The oom killer will kill other processes including getty,klogd,... even root owned ssh.

tixxdz
http://opendz.org

Actually, the original/historical purpose of the exec_id stuff was to protect privileged parent processes (those having done a SUID/SGID exec) from non-standard child exit signals, which could be set with clone(). I think we may want to audit the current implementation and see if it still fully achieves the goal or maybe not (and fix it if not).

IIRC, 32 bits was considered enough because it was only the trusted privileged parent process itself that could potentially cause a wraparound. (I did not verify this conclusion now. It might be wrong.)

I include below pieces of the prototype implementation from linux-2.2.12-ow6.tar.gz released in 1999. One notable difference from the code that went into mainline kernels was that I only incremented the counter on privileged execve(), and I additionally handled counter wraparound. I am a bit concerned that a wraparound attack might be possible on the code currently in mainline kernels, thereby allowing for a bad exit signal to be sent to a privileged new parent program. Does anything prevent the wraparound attack currently? (I did not check for this yet, sorry.)

On exec:

+ bprm->priv_change = id_change || cap_raised;
+ if (bprm->priv_change) {
...
+ /*

+                * Increment the privileged execution counter, so that our
+                * old children know not to send bad exit_signal's to us.
+                * Also, wait on the lock if there's an exit_signal being
+                * sent to us now, to make sure it doesn't get sent to the
+                * new privileged program.
+                */

+ spin_lock_irqsave(&current->priv_lock, flags);
+ if (!++current->priv) {
+ struct task_struct *p;
+
+ /*

+                        * The counter can't really overflow with real-world
+                        * programs (and it has to be the privileged program
+                        * itself that causes the overflow), but we handle
+                        * this case anyway, just for correctness.
+                        */

+ read_lock(&tasklist_lock);
+ for_each_task(p) {
+ if (p->p_pptr == current) {
+ p->ppriv = 0;
+ current->priv = 1;
+ }
+ }
+ read_unlock(&tasklist_lock);
+ }
+ spin_unlock_irqrestore(&current->priv_lock, flags);

In task_struct:

+/* Privileged execution counters, for exit_signal permission checking */
+ spinlock_t priv_lock;
+ int priv, ppriv;

On fork() and clone():

+       spin_lock_init(&p->priv_lock);
+       p->priv = 0;
+       p->ppriv = current->priv;

Exit signal:

+ unsigned long flags = 0;
+ int locked = 0;
+
+ if (sig && sig != SIGCHLD) {
+ /*

+                * Make sure our parent hasn't executed a privileged program
+                * (such as, SUID) since we were born.
+                *
+                * We do some locking here to ensure that there's no race
+                * between the check and actually sending the signal.
+                * Currently, this is probably redundant as notify_parent()
+                * is only used either with the big lock obtained, or with
+                * the signal set to SIGCHLD.
+                */
+               locked = 1;
+               spin_lock_irqsave(&tsk->p_pptr->priv_lock, flags);
+               if (tsk->p_pptr->priv != tsk->ppriv) {
+                       spin_unlock_irqrestore(&tsk->p_pptr->priv_lock, flags);
+                       locked = 0;
+                       sig = 0;

+ }
+ }

...

+ if (locked) spin_unlock_irqrestore(&tsk->p_pptr->priv_lock, flags);

IIRC, an equivalent of the above went upstream (with simplifications and a variables rename by Alan) in 2.2.13, so that may be another "reference implementation" to check against.

Alexander

Funny that, I noticed this message only after I sent the question about the current exec_id stuff.

Perhaps I missed something, but ignoring the "cap_raised" issues, this all is very simple. de_thread() should simply do:

	current->exit_signal = SIGCHLD;

	read_lock(&tasklist_lock);
	list_for_each_entry(p, &current->children, sibling)
		p->exit_signal = SIGCHILD;
	read_unlock(&tasklist_lock);

The only problem is CLONE_PARENT.

Oleg.

So I am dense. /proc/<pid>/mem was special in that it uses a different set of checks than other files, and to do those access checks /proc/<pid>/mem needed to look at exec_id.

For all of the access checks that are not written in that silly way. What is wrong with ptrace_may_access run at every read/write of a file?

We redo all of the permission checks every time so that should avoid races.

I really think you are trying to solve something that is not broken. Certainly I could not see your argument for why anything but /proc/<pid>/mem needs attention.

Eric

If you are referring to the old protection, yes it was against an ID, but not uniq IDs, so you can execve a suid do some tricks to have a match on IDs and bypass the protection, how: by opening your /proc/self/mem and pass the fd to the exec suid who at read/write time will process its own /proc/self/mem

As it was noted, these files change during runtime, so even if you do the ptrace check at each syscall (which is of course a good thing), you must be sure that you are doing the check on the right target and the processed file belongs really to the appropriate process image of the target.

This is what Alan says: "bind objects to their process image".

So if ptrace_may_access is done when current == target then it will succeed, and if we are processing /proc/self/* then every ptrace check will pass, and if you pass the fd of /proc/self/* to a privileged process then it will just operate on its own file.

So we need a way/solution to know that we are processing the _right_ files.

The proposed exec_id here will change on each execve(): uniq ids, so if we execve an external privileged program or have an fd of one of its files, the exec_id check will fail, but we must also perform the ptrace check.

races are against new execves.

IMHO even with that there will be races, here we are operating on virtual files /proc/<pid>/* and the creds and privileges of the process can change at any time even after the ptrace check. Some files use locks see the /proc/<pid>/io commit 293eb1e7772b25a93647c798c7b89bf26c2da2e0

Hope that I'm not out of context.

This patch series is to complete the protection for all the /proc/<pid>/* sensitive files and to update the /proc/<pid>/mem protection. For this last one, the checks were ok, but I thought that we can do better without keeping dead bytes.

For all the thing and to keep track:

This is not news. Alan's historical links:
http://lkml.org/lkml/2012/1/29/35

The previous discussion on kernel-hardening:
(includes some variants described by Vasiliy and other problems which I'll try to discuss here)
http://www.openwall.com/lists/kernel-hardening/2012/02/10/1

These are sensitive files, combined with some binaries properties, attackers can achieve ASLR bypass ...

Thanks Eric.

tixxdz
http://opendz.org

Yes that case is silly and I don't care. I care that you seem to be stomping all over the non-silly cases using the excuse that there was one silly case.

The right target is by definition the current value of the process in question.

/proc/<pid>/<attr> files are supposed to work after an exec.

Adding an exec_id to additional files simply breaks existing applications for no good reason.

What is needed is for safety is to guard against the race of exec happening during a read or a write, so that we don't get access to something we shouldn't have permissions to.

In general that means reference counting or locks. All exec_id can meaningfully be used for in the general case is a trigger to try again.

Alan's case refers to how to handle /proc/<pid>/maps the right way.

In your post on openwall I just see you arguing that the current and deliberate semantics of the permission checks on the proc files are wrong. Because the permission checks happen at access time rather than at open time.

Well I am sorry. The permission checks have happened at access time for ages and that is deliberate.

Furthermore one of your confused arguments seems to imply that there is a such a thing as a /proc/self file distinct from a /proc/<pid> file. There not. /proc/self is just a symlink into /proc/<pid> and as such does not open new security holes.

If you expect /proc/ not to let you find out things about yourself if you are a suid executable that just seems silly.

So in short you seem to be arguing for changing the semantics of access of every file under /proc/<pid> because there is cognitive dissonance between your understanding of those files and what is implemented. I see no acknowledgement that you are arguing for a semantic change or any arguments in favor of that change. At best I see is an argument that says you the files don't work the way you expect which is most definitely not sufficient for a change.

Eric

I'm not sure I follow you here, these proc files suffer from the same problems, can you please point what are these non-silly case ?

Yes.

Can you please point these applications that this patch will break ? So we do not do it.

I've tested 'ps' but perhaps I've missed something ?

We just return 0 at read() which is a correct thing to do.

before and during. I say before since this is what we are tying to emulate.

Locks ? counting yes this perhaps can work as Alan suggested, but a simple check will catch all the things without any node list nor count inc/dec.

Linus's /proc/pid/mem patch do not even do that, he just keep the old mm at open time.

Alan's case refers to how to handle all the /proc/<pid>/* files and any other object that should be attached to its process image.

Yes if you are speaking about {maps,smaps...} then it should also be at open time and at each syscall (it depends on files) and for the /proc/pid/{maps,smaps,numa_maps} yes it is not safe to give non-privileged processes an fd to an objects attached to a privileged process.

Yes (perhaps even before I use Linux :) ), but IMHO they are not perfect, if they were, then we'll not see all the /proc/<pid>/ problems.

The current patches protect /proc/pid/{maps,smaps,numa_maps} without breaking things, there are no checks at open which is not safe, but I did not add it since I was not sure and I don't want to break things (glibc FORITFY_SOURCE ...).

My patches add the ptrace checks at open only for /proc/<pid>/{environ,pagemap,mem} which is safe, I did not include /proc/pid/{auxv,maps,smaps,...} so please if you have _real_ cases of problems just post them.

No it's not confused, we just use the /proc/self as it is easy to explain. Sorry if you feel that.

Ah yes this one about suid/privileged, if you are still privileged then there is no harm, but if you drop your privileges IMHO that means that you do not want to do privileged operations, but that current == task in ptrace which is before the creds check will just allow you to do so.

Well, this is another subject.

I really don't understant what you are trying to say/prove here, this is the same issue of the last /proc/<pid>/mem one that got fixed by Linus and Olge patches. These are dynamic files.

For arguments I'll re-try.

Thanks Eric.

tixxdz
http://opendz.org

As I read 3.3-rc1 which I have handy I do not see problems.

An exec_id check adds no value, to files like /proc/pid/environ. mm_for_maps nicely does the necessary permission checks and avoids the race.

Then the permission checks at access time are broken, and exec_id nonsense won't make them better.

We see problems because we lots of people play in the code and are not careful. It looks to me like you are trying to make the problem worse by making the code more complicated and even harder to understand.

You are thrashing the code and making it more complicated for no apparent reason.

Please show me how you can drop privileges to access yourself.

The current == task check is there because of extreme silliness in the security modules and the fact that glibc breaks in entertaining was if it can not access /proc/self/masp. If you I read you right you want to reintroduce that breaking for suid exectuables. Frankly that sounds like you a security bug in the making.

The /proc/<pid>/mem permission checks were implemented in a very stupid way. In that stupid way they used task->exec_id. I don't see any evidence that anyone touching any of the other proc files made that same foolish set of choices. So I see no evidence that the problem exists outside of anywhere except /proc/<pid>/mem.

So very simply it looks to me like your entire patchset is very busily trying to fix problems that don't exist.

Eric