Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1233957 > unrolled thread
| Started by | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| First post | 2015-09-28 11:10 +0200 |
| Last post | 2015-09-30 10:10 +0200 |
| Articles | 10 on this page of 30 — 8 participants |
Back to article view | Back to linux.kernel
[PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 11:10 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 11:40 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 11:50 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 12:30 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 12:40 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 13:00 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 12:00 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 12:40 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-09-28 17:50 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 18:10 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-09-28 18:40 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-09-29 20:20 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-09-29 20:40 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-29 20:50 +0200
[PATCH] fs/proc: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-09-30 09:20 +0200
Re: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 09:40 +0200
[PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-09-30 16:00 +0200
Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 22:40 +0200
Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan Kees Cook <keescook@chromium.org> - 2015-09-30 23:30 +0200
Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 23:40 +0200
[PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 10:00 +0200
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-01 11:00 +0200
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 11:30 +0200
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-01 12:20 +0200
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 12:40 +0200
Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-01 12:50 +0200
[PATCH v5] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 13:00 +0200
[PATCH v4] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 11:40 +0200
[tip:core/debug] fs/proc, core/debug: Don' t expose absolute kernel addresses via wchan tip-bot for Ingo Molnar <tipbot@zytor.com> - 2015-10-01 15:00 +0200
Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 10:10 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-10-01 10:00 +0200 |
| Subject | [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeGWu-5Xc-21@gated-at.bofh.it> |
| In reply to | #1236772 |
* Kees Cook <keescook@chromium.org> wrote:
> > @@ -507,7 +505,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 */
>
> Probably should also update Documentation/filesystems/proc.txt with
> something like:
>
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
> blocked bitmap of blocked signals
> sigign bitmap of ignored signals
> sigcatch bitmap of caught signals
> - wchan address where process went to sleep
> + 0 (place holder, was wchan, see /proc/PID/wchan instead)
> 0 (place holder)
> 0 (place holder)
> exit_signal signal to send to parent thread on exit
Indeed - I ended up clarifying both wchan explanations, see the changes below.
I also made the 'no symbols' output "0" (instead of an empty string), to better
match the /proc/PID/stat behavior and previous output.
I'll push it out after a bit more testing and if nothing goes wrong I'll send this
patch to Linus in the v4.4 merge window.
Thanks,
Ingo
============>
From bc43bb95763e5b215e389f75860eca0952ca4704 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 30 Sep 2015 15:59:17 +0200
Subject: [PATCH] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field) leaks absolute kernel
addresses to unprivileged user-space, of kernel functions that sleep:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan, if KALLSYMS is
turned off or if the symbol lookup fails for some reason:
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;
}
This isn't ideal, because 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
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.
So this patch sets all numeric output to "0" and keeps only symbolic
output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@vger.kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20150930135917.GA3285@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
Documentation/filesystems/proc.txt | 5 +++--
fs/proc/array.c | 6 ++----
fs/proc/base.c | 9 +++------
3 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index d411ca63c8b6..db64f7d6492d 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
stat Process status
statm Process memory status information
status Process status in human readable form
- wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
+ wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is
+ blocked in) symbol string. "0" if not blocked or !KALLSYMS.
pagemap Page table
stack Report full stack trace, enable via CONFIG_STACKTRACE
smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
blocked bitmap of blocked signals
sigign bitmap of ignored signals
sigcatch bitmap of caught signals
- wchan address where process went to sleep
+ 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead)
0 (place holder)
0 (place holder)
exit_signal signal to send to parent thread on exit
diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f0121e331..ad5ad1e376ad 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task, int whole)
{
- unsigned long vsize, eip, esp, wchan = ~0UL;
+ unsigned long vsize, eip, esp;
int priority, nice;
int tty_pgrp = -1, tty_nr = 0;
sigset_t sigign, sigcatch;
@@ -454,8 +454,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
unlock_task_sighand(task, &flags);
}
- if (permitted && (!whole || num_threads < 2))
- wchan = get_wchan(task);
if (!whole) {
min_flt = task->min_flt;
maj_flt = task->maj_flt;
@@ -507,7 +505,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..6f05aabce3aa 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,10 @@ 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);
- }
+ else
+ seq_putc(m, '0');
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <ryabinin.a.a@gmail.com> |
|---|---|
| Date | 2015-10-01 11:00 +0200 |
| Subject | Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeHSy-7yF-5@gated-at.bofh.it> |
| In reply to | #1237106 |
2015-10-01 10:57 GMT+03:00 Ingo Molnar <mingo@kernel.org>:
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index d411ca63c8b6..db64f7d6492d 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
> stat Process status
> statm Process memory status information
> status Process status in human readable form
> - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
> + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is
> + blocked in) symbol string. "0" if not blocked or !KALLSYMS.
/proc/PID/wchan is under #ifdef CONFIG_KALLSYMS.
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b25eee4cead5..6f05aabce3aa 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -430,13 +430,10 @@ 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);
> - }
> + else
> + seq_putc(m, '0');
Maybe we should respect 'kptr_restrict' sysctl when we use '%ps', '%pB' etc.
printk formats (AFAIK %ps just prints address if KALLSYMS=n, or lookup failed).
In that case you could just do 'seq_printf(m, "%ps", wchan)'.
OTOH, %ps, %pS are used mostly in debugging, so investigating some crash
in production kernel with no !KALLSYMS and with kptr_restrict != 0
will be a nightmare.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-10-01 11:30 +0200 |
| Subject | Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeIlA-8lU-13@gated-at.bofh.it> |
| In reply to | #1237156 |
* Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2015-10-01 10:57 GMT+03:00 Ingo Molnar <mingo@kernel.org>:
> > diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> > index d411ca63c8b6..db64f7d6492d 100644
> > --- a/Documentation/filesystems/proc.txt
> > +++ b/Documentation/filesystems/proc.txt
> > @@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
> > stat Process status
> > statm Process memory status information
> > status Process status in human readable form
> > - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
> > + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is
> > + blocked in) symbol string. "0" if not blocked or !KALLSYMS.
>
> /proc/PID/wchan is under #ifdef CONFIG_KALLSYMS.
Yeah, indeed, so I clarified that text to now read:
+ wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function
+ symbol the task is blocked in - or "0" if not blocked.
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index b25eee4cead5..6f05aabce3aa 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -430,13 +430,10 @@ 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);
> > - }
> > + else
> > + seq_putc(m, '0');
>
> Maybe we should respect 'kptr_restrict' sysctl when we use '%ps', '%pB' etc.
> printk formats (AFAIK %ps just prints address if KALLSYMS=n, or lookup failed).
> In that case you could just do 'seq_printf(m, "%ps", wchan)'.
>
> OTOH, %ps, %pS are used mostly in debugging, so investigating some crash in
> production kernel with no !KALLSYMS and with kptr_restrict != 0 will be a
> nightmare.
So this code does not use %pX, it prints the symbol. Yes, the symbol in itself is
'information' about the execution of the task in itself - but /proc per se is all
about providing information about tasks in the system (including to unprivileged
users), so there's IMHO little point in restricting this output any further ...
I think ktrp_restrict is mostly about not exposing absolute addresses.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <ryabinin.a.a@gmail.com> |
|---|---|
| Date | 2015-10-01 12:20 +0200 |
| Subject | Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeJ7Y-13Z-11@gated-at.bofh.it> |
| In reply to | #1237176 |
On 10/01/2015 12:29 PM, Ingo Molnar wrote:
>
> * Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>
>> 2015-10-01 10:57 GMT+03:00 Ingo Molnar <mingo@kernel.org>:
>>> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
>>> index d411ca63c8b6..db64f7d6492d 100644
>>> --- a/Documentation/filesystems/proc.txt
>>> +++ b/Documentation/filesystems/proc.txt
>>> @@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
>>> stat Process status
>>> statm Process memory status information
>>> status Process status in human readable form
>>> - wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
>>> + wchan If CONFIG_KALLSYMS=y, wchan (the kernel function the process is
>>> + blocked in) symbol string. "0" if not blocked or !KALLSYMS.
>>
>> /proc/PID/wchan is under #ifdef CONFIG_KALLSYMS.
>
> Yeah, indeed, so I clarified that text to now read:
>
> + wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function
> + symbol the task is blocked in - or "0" if not blocked.
>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index b25eee4cead5..6f05aabce3aa 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -430,13 +430,10 @@ 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);
>>> - }
>>> + else
>>> + seq_putc(m, '0');
>>
>> Maybe we should respect 'kptr_restrict' sysctl when we use '%ps', '%pB' etc.
>> printk formats (AFAIK %ps just prints address if KALLSYMS=n, or lookup failed).
>> In that case you could just do 'seq_printf(m, "%ps", wchan)'.
>>
>> OTOH, %ps, %pS are used mostly in debugging, so investigating some crash in
>> production kernel with no !KALLSYMS and with kptr_restrict != 0 will be a
>> nightmare.
>
> So this code does not use %pX, it prints the symbol.
I think you misunderstood me.
Yes, this code currently doesn't use %pX, but it could:
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b25eee4..f58f66e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -425,18 +425,7 @@ static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
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);
- }
+ seq_printf(m, "%ps", get_wchan(task));
return 0;
}
There is a problem here, though. %ps will print absolute kernel address instead of symbol name
if KALLSYMS=n or if resolution of address failed.
So I was wondering, may be should just fix %ps ?
i.e. print 0 instead of absolute address if KALLSYMS=n or lookup failure?
> Yes, the symbol in itself is
> 'information' about the execution of the task in itself - but /proc per se is all
> about providing information about tasks in the system (including to unprivileged
> users), so there's IMHO little point in restricting this output any further ...
>
> I think ktrp_restrict is mostly about not exposing absolute addresses.
>
Right, and '%ps' may expose absolute address if KALLSYMS=n or address lookup failed for some reason.
> Thanks,
>
> Ingo
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-10-01 12:40 +0200 |
| Subject | Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeJrk-1qD-19@gated-at.bofh.it> |
| In reply to | #1237250 |
* Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> I think you misunderstood me.
> Yes, this code currently doesn't use %pX, but it could:
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b25eee4..f58f66e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -425,18 +425,7 @@ static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
> 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);
> - }
> + seq_printf(m, "%ps", get_wchan(task));
>
> return 0;
> }
>
>
> There is a problem here, though. %ps will print absolute kernel address instead of symbol name
> if KALLSYMS=n or if resolution of address failed.
> So I was wondering, may be should just fix %ps ?
> i.e. print 0 instead of absolute address if KALLSYMS=n or lookup failure?
There's another problem as well: your change loses the PTRACE_MODE_READ permission
check.
But ... I think I like it open coded, which is good precisely because it will stay
invariant even if we change details in the %ps/etc. debug output.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <ryabinin.a.a@gmail.com> |
|---|---|
| Date | 2015-10-01 12:50 +0200 |
| Subject | Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeJB0-1BQ-7@gated-at.bofh.it> |
| In reply to | #1237258 |
2015-10-01 13:39 GMT+03:00 Ingo Molnar <mingo@kernel.org>: > > There's another problem as well: your change loses the PTRACE_MODE_READ permission > check. I guess, you should fix your patch than, because it removed this check likewise... > > But ... I think I like it open coded, which is good precisely because it will stay > invariant even if we change details in the %ps/etc. debug output. > Fair enough. > Thanks, > > Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-10-01 13:00 +0200 |
| Subject | [PATCH v5] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeJKH-1N3-25@gated-at.bofh.it> |
| In reply to | #1237260 |
* Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2015-10-01 13:39 GMT+03:00 Ingo Molnar <mingo@kernel.org>:
> >
> > There's another problem as well: your change loses the PTRACE_MODE_READ permission
> > check.
>
> I guess, you should fix your patch than, because it removed this check
> likewise...
Doh, indeed. I didn't notice that bug in testing, because the wchan field of
/proc/PID/stat was 0/1 according to the ptrace check so 'ps' masked the symbol
accordingly - but the /proc/PID/wchan value was always accessible.
Updated patch below.
Thanks,
Ingo
=======================>
From b2f73922d119686323f14fbbe46587f863852328 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 30 Sep 2015 15:59:17 +0200
Subject: [PATCH] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field, which contains
the absolute kernel address of the kernel function a task is blocked in)
leaks absolute kernel addresses to unprivileged user-space:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan as well, if
KALLSYMS is turned off or if the symbol lookup fails for some reason:
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;
}
This isn't ideal, because 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
There's one compatibility quirk here: procps relies on whether the
absolute value is non-zero - and we can provide that functionality
by outputing "0" or "1" depending on whether the task is blocked
(whether there's a wchan address).
These days there appears to be 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.
So this patch sets all numeric output to "0" or "1" and keeps only
symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@vger.kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20150930135917.GA3285@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
Documentation/filesystems/proc.txt | 5 +++--
fs/proc/array.c | 16 ++++++++++++++--
fs/proc/base.c | 9 +++------
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index d411ca63c8b6..3a9d65c912e7 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
stat Process status
statm Process memory status information
status Process status in human readable form
- wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
+ wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function
+ symbol the task is blocked in - or "0" if not blocked.
pagemap Page table
stack Report full stack trace, enable via CONFIG_STACKTRACE
smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
blocked bitmap of blocked signals
sigign bitmap of ignored signals
sigcatch bitmap of caught signals
- wchan address where process went to sleep
+ 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead)
0 (place holder)
0 (place holder)
exit_signal signal to send to parent thread on exit
diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f0121e331..eed2050db9be 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task, int whole)
{
- unsigned long vsize, eip, esp, wchan = ~0UL;
+ unsigned long vsize, eip, esp, wchan = 0;
int priority, nice;
int tty_pgrp = -1, tty_nr = 0;
sigset_t sigign, sigcatch;
@@ -507,7 +507,19 @@ 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);
+
+ /*
+ * We used to output the absolute kernel address, but that's an
+ * information leak - so instead we show a 0/1 flag here, to signal
+ * to user-space whether there's a wchan field in /proc/PID/wchan.
+ *
+ * This works with older implementations of procps as well.
+ */
+ if (wchan)
+ seq_puts(m, " 1");
+ else
+ seq_puts(m, " 0");
+
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..29595af32866 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,10 @@ 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 (wchan && ptrace_may_access(task, PTRACE_MODE_READ) && !lookup_symbol_name(wchan, symname))
seq_printf(m, "%s", symname);
- }
+ else
+ seq_putc(m, '0');
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-10-01 11:40 +0200 |
| Subject | [PATCH v4] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan |
| Message-ID | <qeIvk-5L-99@gated-at.bofh.it> |
| In reply to | #1237106 |
* Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
> > > @@ -507,7 +505,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 */
> >
> > Probably should also update Documentation/filesystems/proc.txt with
> > something like:
> >
> > --- a/Documentation/filesystems/proc.txt
> > +++ b/Documentation/filesystems/proc.txt
> > @@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
> > blocked bitmap of blocked signals
> > sigign bitmap of ignored signals
> > sigcatch bitmap of caught signals
> > - wchan address where process went to sleep
> > + 0 (place holder, was wchan, see /proc/PID/wchan instead)
> > 0 (place holder)
> > 0 (place holder)
> > exit_signal signal to send to parent thread on exit
>
> Indeed - I ended up clarifying both wchan explanations, see the changes below.
>
> I also made the 'no symbols' output "0" (instead of an empty string), to better
> match the /proc/PID/stat behavior and previous output.
>
> I'll push it out after a bit more testing and if nothing goes wrong I'll send this
> patch to Linus in the v4.4 merge window.
Yeah, so testing uncovered the following additional ABI detail: procps relies on
the wchan field in /proc/PID/stat, but only as a flag (in most cases), whether to
look at /proc/PID/wchan.
To keep the ABI, the v4 patch below outputs not the absolute address, but a 0/1
flag to indicate whether the task is blocked and whether there's anything worth
looking at in /proc/PID/wchan.
I tested this approach with procps and it seems to fully work. In fact due to the
ptrace check we properly restrict the information to our own tasks only. root
still sees the wchan field of all tasks.
Btw., the very latest procps-ng grew this nice change:
6b8dc5511fb9 ("library: refactor and rely on modern kernels for wchan")
which greatly simplified procps's handling of /proc/PID/wchan.
... but my testing was done with an older procps version.
Thanks,
Ingo
==========================>
From b26a16469b0b6f3f0604aafb95c50d1532b3fff2 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 30 Sep 2015 15:59:17 +0200
Subject: [PATCH] fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field, which contains
the absolute kernel address of the kernel function a task is blocked in)
leaks absolute kernel addresses to unprivileged user-space:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan as well, if
KALLSYMS is turned off or if the symbol lookup fails for some reason:
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;
}
This isn't ideal, because 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
There's one compatibility quirk here: procps relies on whether the
absolute value is non-zero - and we can provide that functionality
by outputing "0" or "1" depending on whether the task is blocked
(whether there's a wchan address).
These days there appears to be 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.
So this patch sets all numeric output to "0" or "1" and keeps only
symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@vger.kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20150930135917.GA3285@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
Documentation/filesystems/proc.txt | 5 +++--
fs/proc/array.c | 16 ++++++++++++++--
fs/proc/base.c | 9 +++------
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index d411ca63c8b6..3a9d65c912e7 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
stat Process status
statm Process memory status information
status Process status in human readable form
- wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
+ wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function
+ symbol the task is blocked in - or "0" if not blocked.
pagemap Page table
stack Report full stack trace, enable via CONFIG_STACKTRACE
smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
blocked bitmap of blocked signals
sigign bitmap of ignored signals
sigcatch bitmap of caught signals
- wchan address where process went to sleep
+ 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead)
0 (place holder)
0 (place holder)
exit_signal signal to send to parent thread on exit
diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f0121e331..eed2050db9be 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task, int whole)
{
- unsigned long vsize, eip, esp, wchan = ~0UL;
+ unsigned long vsize, eip, esp, wchan = 0;
int priority, nice;
int tty_pgrp = -1, tty_nr = 0;
sigset_t sigign, sigcatch;
@@ -507,7 +507,19 @@ 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);
+
+ /*
+ * We used to output the absolute kernel address, but that's an
+ * information leak - so instead we show a 0/1 flag here, to signal
+ * to user-space whether there's a wchan field in /proc/PID/wchan.
+ *
+ * This works with older implementations of procps as well.
+ */
+ if (wchan)
+ seq_puts(m, " 1");
+ else
+ seq_puts(m, " 0");
+
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..6f05aabce3aa 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,10 @@ 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);
- }
+ else
+ seq_putc(m, '0');
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Ingo Molnar <tipbot@zytor.com> |
|---|---|
| Date | 2015-10-01 15:00 +0200 |
| Subject | [tip:core/debug] fs/proc, core/debug: Don' t expose absolute kernel addresses via wchan |
| Message-ID | <qeLCO-4xp-31@gated-at.bofh.it> |
| In reply to | #1236322 |
Commit-ID: b2f73922d119686323f14fbbe46587f863852328
Gitweb: http://git.kernel.org/tip/b2f73922d119686323f14fbbe46587f863852328
Author: Ingo Molnar <mingo@kernel.org>
AuthorDate: Wed, 30 Sep 2015 15:59:17 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 1 Oct 2015 12:55:34 +0200
fs/proc, core/debug: Don't expose absolute kernel addresses via wchan
So the /proc/PID/stat 'wchan' field (the 30th field, which contains
the absolute kernel address of the kernel function a task is blocked in)
leaks absolute kernel addresses to unprivileged user-space:
seq_put_decimal_ull(m, ' ', wchan);
The absolute address might also leak via /proc/PID/wchan as well, if
KALLSYMS is turned off or if the symbol lookup fails for some reason:
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;
}
This isn't ideal, because 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
There's one compatibility quirk here: procps relies on whether the
absolute value is non-zero - and we can provide that functionality
by outputing "0" or "1" depending on whether the task is blocked
(whether there's a wchan address).
These days there appears to be 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.
So this patch sets all numeric output to "0" or "1" and keeps only
symbolic output, in /proc/PID/wchan.
( The absolute sleep address can generally still be profiled via
perf, by tasks with sufficient privileges. )
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@vger.kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20150930135917.GA3285@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
Documentation/filesystems/proc.txt | 5 +++--
fs/proc/array.c | 16 ++++++++++++++--
fs/proc/base.c | 9 +++------
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index d411ca6..3a9d65c 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -140,7 +140,8 @@ Table 1-1: Process specific entries in /proc
stat Process status
statm Process memory status information
status Process status in human readable form
- wchan If CONFIG_KALLSYMS is set, a pre-decoded wchan
+ wchan Present with CONFIG_KALLSYMS=y: it shows the kernel function
+ symbol the task is blocked in - or "0" if not blocked.
pagemap Page table
stack Report full stack trace, enable via CONFIG_STACKTRACE
smaps a extension based on maps, showing the memory consumption of
@@ -310,7 +311,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
blocked bitmap of blocked signals
sigign bitmap of ignored signals
sigcatch bitmap of caught signals
- wchan address where process went to sleep
+ 0 (place holder, used to be the wchan address, use /proc/PID/wchan instead)
0 (place holder)
0 (place holder)
exit_signal signal to send to parent thread on exit
diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f012..eed2050 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task, int whole)
{
- unsigned long vsize, eip, esp, wchan = ~0UL;
+ unsigned long vsize, eip, esp, wchan = 0;
int priority, nice;
int tty_pgrp = -1, tty_nr = 0;
sigset_t sigign, sigcatch;
@@ -507,7 +507,19 @@ 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);
+
+ /*
+ * We used to output the absolute kernel address, but that's an
+ * information leak - so instead we show a 0/1 flag here, to signal
+ * to user-space whether there's a wchan field in /proc/PID/wchan.
+ *
+ * This works with older implementations of procps as well.
+ */
+ if (wchan)
+ seq_puts(m, " 1");
+ else
+ seq_puts(m, " 0");
+
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 b25eee4..29595af 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,10 @@ 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 (wchan && ptrace_may_access(task, PTRACE_MODE_READ) && !lookup_symbol_name(wchan, symname))
seq_printf(m, "%s", symname);
- }
+ else
+ seq_putc(m, '0');
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2015-09-30 10:10 +0200 |
| Message-ID | <qekCD-7xs-31@gated-at.bofh.it> |
| In reply to | #1235461 |
On Tue, 29 Sep 2015, Andy Lutomirski wrote: > I'm be vaguely amazed if this isn't an exploitable info leak even > without the out of bounds thing. The info leak happens in fs/proc, where we happily print arbitrary "IP" values, if we cant resolve a symbol. > Can we really not find a way to do this without walking the stack? We would have to add a 'store wait channel' mechanism to all functions which are the primary entry points to scheduling. Not impossible, but not pretty either. If we want to prevent the stack changing under us, we'd need to take p->pi_lock and do the task != RUNNING check and the walk under it. I don't think we want to do that, unless there is a compelling reason to do so. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web