* Re: [PATCH] proc: restrict access to /proc/$PID/{sched,schedstat} [not found] <20111105104828.GA3489@albatros> @ 2011-11-08 11:59 ` Vasiliy Kulikov 2011-11-08 23:17 ` Andrew Morton 0 siblings, 1 reply; 3+ messages in thread From: Vasiliy Kulikov @ 2011-11-08 11:59 UTC (permalink / raw) To: Andrew Morton, linux-kernel Cc: Al Viro, Stephen Wilson, Alexey Dobriyan, security (CC'ed l-k) On Sat, Nov 05, 2011 at 14:48 +0400, Vasiliy Kulikov wrote: > /proc/$PID/{sched,schedstat} contain debugging scheduler counters, which > should not be world readable. They may be used to gather private information > about processes' activity. E.g. it can be used to count the number of > characters typed in gksu dialog: > > http://www.openwall.com/lists/oss-security/2011/11/05/3 > > This infoleak is similar to io (1d1221f375c) and stat's eip/esp (f83ce3e6b02d) > infoleaks. Probably other 0644/0444 procfs files are vulnerable to > similar infoleaks. > > Cc: <stable@kernel.org> > Signed-off-by: Vasiliy Kulikov <segoon@openwall.com> > --- > fs/proc/base.c | 32 ++++++++++++++++++++++---------- > 1 files changed, 22 insertions(+), 10 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 6278ef1..8b67eec 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -410,10 +410,16 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns, > */ > static int proc_pid_schedstat(struct task_struct *task, char *buffer) > { > - return sprintf(buffer, "%llu %llu %lu\n", > - (unsigned long long)task->se.sum_exec_runtime, > - (unsigned long long)task->sched_info.run_delay, > - task->sched_info.pcount); > + int ret; > + ret = lock_trace(task); > + if (!ret) { > + ret = sprintf(buffer, "%llu %llu %lu\n", > + (unsigned long long)task->se.sum_exec_runtime, > + (unsigned long long)task->sched_info.run_delay, > + task->sched_info.pcount); > + unlock_trace(task); > + } > + return ret; > } > #endif > > @@ -1390,15 +1396,21 @@ static int sched_show(struct seq_file *m, void *v) > { > struct inode *inode = m->private; > struct task_struct *p; > + int ret; > > p = get_proc_task(inode); > if (!p) > return -ESRCH; > - proc_sched_show_task(p, m); > + ret = lock_trace(p); > + if (!ret) { > + proc_sched_show_task(p, m); > + ret = 0; > + unlock_trace(p); > + } > > put_task_struct(p); > > - return 0; > + return ret; > } > > static ssize_t > @@ -2813,7 +2825,7 @@ static const struct pid_entry tgid_base_stuff[] = { > ONE("personality", S_IRUGO, proc_pid_personality), > INF("limits", S_IRUGO, proc_pid_limits), > #ifdef CONFIG_SCHED_DEBUG > - REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations), > + REG("sched", S_IRUSR|S_IWUSR, proc_pid_sched_operations), > #endif > #ifdef CONFIG_SCHED_AUTOGROUP > REG("autogroup", S_IRUGO|S_IWUSR, proc_pid_sched_autogroup_operations), > @@ -2851,7 +2863,7 @@ static const struct pid_entry tgid_base_stuff[] = { > ONE("stack", S_IRUGO, proc_pid_stack), > #endif > #ifdef CONFIG_SCHEDSTATS > - INF("schedstat", S_IRUGO, proc_pid_schedstat), > + INF("schedstat", S_IRUSR, proc_pid_schedstat), > #endif > #ifdef CONFIG_LATENCYTOP > REG("latency", S_IRUGO, proc_lstats_operations), > @@ -3162,7 +3174,7 @@ static const struct pid_entry tid_base_stuff[] = { > ONE("personality", S_IRUGO, proc_pid_personality), > INF("limits", S_IRUGO, proc_pid_limits), > #ifdef CONFIG_SCHED_DEBUG > - REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations), > + REG("sched", S_IRUSR|S_IWUSR, proc_pid_sched_operations), > #endif > REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations), > #ifdef CONFIG_HAVE_ARCH_TRACEHOOK > @@ -3196,7 +3208,7 @@ static const struct pid_entry tid_base_stuff[] = { > ONE("stack", S_IRUGO, proc_pid_stack), > #endif > #ifdef CONFIG_SCHEDSTATS > - INF("schedstat", S_IRUGO, proc_pid_schedstat), > + INF("schedstat", S_IRUSR, proc_pid_schedstat), > #endif > #ifdef CONFIG_LATENCYTOP > REG("latency", S_IRUGO, proc_lstats_operations), > -- > 1.7.0.4 > -- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] proc: restrict access to /proc/$PID/{sched,schedstat} 2011-11-08 11:59 ` [PATCH] proc: restrict access to /proc/$PID/{sched,schedstat} Vasiliy Kulikov @ 2011-11-08 23:17 ` Andrew Morton 2011-11-09 9:06 ` Vasiliy Kulikov 0 siblings, 1 reply; 3+ messages in thread From: Andrew Morton @ 2011-11-08 23:17 UTC (permalink / raw) To: Vasiliy Kulikov Cc: linux-kernel, Al Viro, Stephen Wilson, Alexey Dobriyan, security On Tue, 8 Nov 2011 15:59:00 +0400 Vasiliy Kulikov <segoon@openwall.com> wrote: > (CC'ed l-k) security@kernel.org isn't working, btw. > On Sat, Nov 05, 2011 at 14:48 +0400, Vasiliy Kulikov wrote: > > /proc/$PID/{sched,schedstat} contain debugging scheduler counters, which > > should not be world readable. They may be used to gather private information > > about processes' activity. E.g. it can be used to count the number of > > characters typed in gksu dialog: > > > > http://www.openwall.com/lists/oss-security/2011/11/05/3 > > > > This infoleak is similar to io (1d1221f375c) and stat's eip/esp (f83ce3e6b02d) > > infoleaks. Probably other 0644/0444 procfs files are vulnerable to > > similar infoleaks. Grumble. The obvious issue with this patch is its non-back-compatibility. What existing code will break, in what manner and what is the seriousness of the breakage? You *know* this is the main issue, yet you didn't address it at all! You just leave the issue out there for other people to work out, and to ask the obvious questions. This happens over and over and I'm getting rather tired of the charade. So I'm going to ignore this patch and I ask that you and other security people never do this again. If you're going to submit a patch which you know will change kernel interfaces in a non-backward-compatible fashion then don't just pretend that it didn't happen! Please provide us with a complete description of the breakage and at least some analysis of the downstream implications of the change. So that we are better able to decide whether the security improvements justify the disruption. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] proc: restrict access to /proc/$PID/{sched,schedstat} 2011-11-08 23:17 ` Andrew Morton @ 2011-11-09 9:06 ` Vasiliy Kulikov 0 siblings, 0 replies; 3+ messages in thread From: Vasiliy Kulikov @ 2011-11-09 9:06 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Al Viro, Stephen Wilson, Alexey Dobriyan, security, kernel-hardening On Tue, Nov 08, 2011 at 15:17 -0800, Andrew Morton wrote: > > On Sat, Nov 05, 2011 at 14:48 +0400, Vasiliy Kulikov wrote: > > > /proc/$PID/{sched,schedstat} contain debugging scheduler counters, which > > > should not be world readable. They may be used to gather private information > > > about processes' activity. E.g. it can be used to count the number of > > > characters typed in gksu dialog: > > > > > > http://www.openwall.com/lists/oss-security/2011/11/05/3 > > > > > > This infoleak is similar to io (1d1221f375c) and stat's eip/esp (f83ce3e6b02d) > > > infoleaks. Probably other 0644/0444 procfs files are vulnerable to > > > similar infoleaks. > > Grumble. > > The obvious issue with this patch is its non-back-compatibility. What > existing code will break, in what manner and what is the seriousness of > the breakage? > > You *know* this is the main issue, yet you didn't address it at all! > You just leave the issue out there for other people to work out, and to > ask the obvious questions. > > This happens over and over and I'm getting rather tired of the charade. > > So I'm going to ignore this patch and I ask that you and other security > people never do this again. > > If you're going to submit a patch which you know will change kernel > interfaces in a non-backward-compatible fashion then don't just pretend > that it didn't happen! Please provide us with a complete description > of the breakage and at least some analysis of the downstream > implications of the change. So that we are better able to decide > whether the security improvements justify the disruption. I'm sorry it looked like I didn't test the patch, but I really didn't face to any breakage (top, ps, gnome monitor). The actual problem is that the patch is still incomplete - all proc monitoring tools watch for /proc/$PID/stat file content changes, not /proc/$PID/sched. /proc/$PID/stat contains the same sched information, which I've missed. Restricting "stat" does break these tools. /proc/$PID/stat already has "fake" fields like KSTK_EIP() and KSTK_ESP(). We can continue to do such sort of force fields zeroing, which doesn't break ABI. Thanks, -- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-11-09 9:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20111105104828.GA3489@albatros>
2011-11-08 11:59 ` [PATCH] proc: restrict access to /proc/$PID/{sched,schedstat} Vasiliy Kulikov
2011-11-08 23:17 ` Andrew Morton
2011-11-09 9:06 ` Vasiliy Kulikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox