proc: fix a race in do_io_accounting()

There is a ptrace_may_access() check in do_io_accounting() to prevent gathering information of setuid'ed and similar binaries. However, there is a race against execve(). Holding task->signal->cred_guard_mutex while gathering the information should protect against the race.

The order of locking is similar to the one inside of ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

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

index 083a4f2..c605ee1 100644
@@ -2711,9 +2711,12 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 {
 	struct task_io_accounting acct = task->ioac;
 	unsigned long flags;
+	int result = -EACCES;
+
+	mutex_lock(&task->signal->cred_guard_mutex);
 
 	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		return -EACCES;
+		goto out_unlock;
 
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;

@@ -2724,7 +2727,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 
 		unlock_task_sighand(task, &flags);
 	}
-	return sprintf(buffer,
+	result = sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
 			"syscr: %llu\n"

@@ -2739,6 +2742,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
+out_unlock:
+	mutex_unlock(&task->signal->cred_guard_mutex);
+	return result;
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)

1.7.0.4

Hmm. mm_for_maps() uses mutex_lock_killable(), as does lock_trace.

And neither proc_pid_wchan() nor the fd following ones (proc_pid_follow_link etc) use anything at all.

So I'm not sure. And do we really even care about the theoretical race? Even if we do hit the race window and happen to get it just as a process turns setuid, it would seem to be totally harmless (we're not going to see any of the sensitive IO anyway).

So considering the lack of consistency in this area, I can't really find it in myself to care very deeply.

That said, the lack of consistency itself is a bit annoying and worrisome. Maybe some kind of helper like we do have for "mm_for_maps()" would be a good idea - not because the potential races are all that worrisome, but because inconsistencies in the kernel are always signs of confusion, and confusion is always bad and a breeding ground for potential bugs.

Linus

Killable/interruptable here makes sense.

I consider this as a theoretical race too unless there is a crazy bug in scheduler/timer. But IMO it's better to just fully remove the risk (even purely theoretical) given the lock is simple and it doesn't cost much.

Thanks,

Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

There is a ptrace_may_access() check in do_io_accounting() to prevent gathering information of setuid'ed and similar binaries. However, there is a race against execve(). Holding task->signal->cred_guard_mutex while gathering the information should protect against the race.

The order of locking is similar to the one inside of ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

v2 - use mutex_lock_killable() instead of mutex_lock().

---
 fs/proc/base.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

index 083a4f2..4b9f159 100644
@@ -2711,9 +2711,16 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 {
 	struct task_io_accounting acct = task->ioac;
 	unsigned long flags;
+	int result;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		return -EACCES;
+	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (result)
+		return result;
+
+	if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+		result = -EACCES;
+		goto out_unlock;
+	}
 
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;

@@ -2724,7 +2731,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 
 		unlock_task_sighand(task, &flags);
 	}
-	return sprintf(buffer,
+	result = sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
 			"syscr: %llu\n"

@@ -2739,6 +2746,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
+out_unlock:
+	mutex_unlock(&task->signal->cred_guard_mutex);
+	return result;
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)

1.7.0.4

<wakes up>

If a patch is to be backported into -stable then its changelog had better explain why such a thing is needed. This one doesn't.

Please provide a full description of the conseuqences of the bug. One which will permit the -stable maintainers to understand why they're merging the patch, and one which will help distribution maintainers decide whether they want to merge it as well.

If inode's mode permits to open /proc/PID/io and the resulted file descriptor is kept across execve() of setuid or similar binary, the ptrace_may_access() check tries to prevent using this fd against the task with escalated privileges. Unfortunately, there is a race of the check against execve(). If execve() is processed after the ptrace check, but before the actual io information gathering, io statistics will be gathered from the privileged process. At least in theory this might lead to gathering sensible information (like ssh/ftp password length) that wouldn't be available otherwise.

Holding task->signal->cred_guard_mutex while gathering the io information should protect against the race.

The order of locking is similar to the one inside of ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand().

v3 - better description.
v2 - use mutex_lock_killable() instead of mutex_lock().

---
 fs/proc/base.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

index 083a4f2..4b9f159 100644
@@ -2711,9 +2711,16 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 {
 	struct task_io_accounting acct = task->ioac;
 	unsigned long flags;
+	int result;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		return -EACCES;
+	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (result)
+		return result;
+
+	if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+		result = -EACCES;
+		goto out_unlock;
+	}
 
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;

@@ -2724,7 +2731,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 
 		unlock_task_sighand(task, &flags);
 	}
-	return sprintf(buffer,
+	result = sprintf(buffer,
 			"rchar: %llu\n"
 			"wchar: %llu\n"
 			"syscr: %llu\n"

@@ -2739,6 +2746,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
+out_unlock:
+	mutex_unlock(&task->signal->cred_guard_mutex);
+	return result;
 }
 
 static int proc_tid_io_accounting(struct task_struct *task, char *buffer)

1.7.0.4

Hi Linus,

Any problems with the patch?

Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

I still think that the problem you fixed is simply not important, and the problem that I actually might care about (the ad-hoc and inconsistent rules) remains.

So the patch is just a "not important enough to care about" for me, and if it comes in through Andrew I'll probably take it, but it's merge-window material rather than anything else.

Linus