| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150930071537.GA19048@gmail.com>
Date: Wed, 30 Sep 2015 09:15:37 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Andy Lutomirski <luto@...capital.net>,
Kees Cook <keescook@...omium.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Dmitry Vyukov <dvyukov@...gle.com>,
Andrey Ryabinin <ryabinin.a.a@...il.com>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
Andy Lutomirski <luto@...nel.org>,
Borislav Petkov <bp@...en8.de>,
Denys Vlasenko <dvlasenk@...hat.com>,
"x86@...nel.org" <x86@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Kostya Serebryany <kcc@...gle.com>,
Alexander Potapenko <glider@...gle.com>,
Andrey Konovalov <andreyknvl@...gle.com>,
Sasha Levin <sasha.levin@...cle.com>,
Andi Kleen <ak@...ux.intel.com>,
kasan-dev <kasan-dev@...glegroups.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Andrew Morton <akpm@...ux-foundation.org>,
Kees Cook <keescook@...omium.org>,
Al Viro <viro@...iv.linux.org.uk>
Subject: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan
* Andy Lutomirski <luto@...capital.net> wrote:
> > + * ----------- bottom = start + sizeof(thread_info)
> > + * thread_info
> > + * ----------- start
> > + *
> > + * The tasks stack pointer points at the location where the
> > + * framepointer is stored. The data on the stack is:
> > + * ... IP FP ... IP FP
> > + *
> > + * We need to read FP and IP, so we need to adjust the upper
> > + * bound by another unsigned long.
> > + */
> > + top = start + THREAD_SIZE - 2 * sizeof(unsigned long);
> > + bottom = start + sizeof(struct thread_info);
> > +
> > + sp = p->thread.sp;
> > + if (sp < bottom || sp > top)
> > + return 0;
> > +
> > + fp = *(unsigned long *)sp;
> > do {
> > - if (fp < (unsigned long)stack ||
> > - fp >= (unsigned long)stack+THREAD_SIZE)
> > + if (fp < bottom || fp > top)
> > return 0;
> > - ip = *(u64 *)(fp+8);
> > + ip = *(unsigned long *)(fp + sizeof(unsigned long));
> > if (!in_sched_functions(ip))
> > return ip;
> > - fp = *(u64 *)fp;
> > + fp = *(unsigned long *)fp;
> > } while (count++ < 16);
>
> I'm be vaguely amazed if this isn't an exploitable info leak even
> without the out of bounds thing. Can we really not find a way to do
> this without walking the stack?
So wchan leaks absolute kernel addresses to unprivileged user-space, of kernel
functions that sleep:
static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
unsigned long wchan;
char symname[KSYM_NAME_LEN];
wchan = get_wchan(task);
if (lookup_symbol_name(wchan, symname) < 0) {
if (!ptrace_may_access(task, PTRACE_MODE_READ))
return 0;
seq_printf(m, "%lu", wchan);
} else {
seq_printf(m, "%s", symname);
}
return 0;
}
So for example it trivially leaks the KASLR offset to any local attacker:
fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35)
ffffffff8123b380
Most real-life uses of wchan are symbolic:
ps -eo pid:10,tid:10,wchan:30,comm
and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:
triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
open("/proc/30833/wchan", O_RDONLY) = 6
So shouldn't we try to set all numeric output to 0 and only allow symbolic output
via /proc/PID/wchan?
These days there's very little legitimate reason user-space would be interested in
the absolute address. The absolute address is mostly historic: from the days when
we didn't have kallsyms and user-space procps had to do the decoding itself via
the System.map.
( The absolute sleep address can generally still be profiled via perf, by tasks
with sufficient privileges. )
I.e. how about something like the patch below? (completely untested.)
Thanks,
Ingo
======================>
fs/proc/array.c | 2 +-
fs/proc/base.c | 7 +------
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f0121e331..99082730b2ac 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
- seq_put_decimal_ull(m, ' ', wchan);
+ seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */
seq_put_decimal_ull(m, ' ', 0);
seq_put_decimal_ull(m, ' ', 0);
seq_put_decimal_ll(m, ' ', task->exit_signal);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b25eee4cead5..2fdbf303e3eb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
wchan = get_wchan(task);
- if (lookup_symbol_name(wchan, symname) < 0) {
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
- return 0;
- seq_printf(m, "%lu", wchan);
- } else {
+ if (!lookup_symbol_name(wchan, symname))
seq_printf(m, "%s", symname);
- }
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists