Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1494226 > unrolled thread

[PATCH 0/3] ABI CHANGE!!! Remove questionable remote SP reads

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] ABI CHANGE!!! Remove questionable remote SP reads Andy Lutomirski <luto@kernel.org> - 2016-09-30 20:00 +0200
    [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat Andy Lutomirski <luto@kernel.org> - 2016-09-30 20:00 +0200
      Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat Jann Horn <jann@thejh.net> - 2016-09-30 21:00 +0200
        Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat Andy Lutomirski <luto@amacapital.net> - 2016-10-01 04:10 +0200
          Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-01 06:30 +0200
          Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat Jann Horn <jann@thejh.net> - 2016-10-01 12:40 +0200
    Re: [PATCH 0/3] ABI CHANGE!!! Remove questionable remote SP reads Andy Lutomirski <luto@amacapital.net> - 2016-10-04 01:10 +0200
      Re: [PATCH 0/3] ABI CHANGE!!! Remove questionable remote SP reads Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-04 01:20 +0200
        Re: [PATCH 0/3] ABI CHANGE!!! Remove questionable remote SP reads Raymond Jennings <shentino@gmail.com> - 2016-10-04 09:10 +0200

#1494226 — [PATCH 0/3] ABI CHANGE!!! Remove questionable remote SP reads

Jann Horn noticed that KSTK_ESP + eager task stack freeing was a bad
combination and could crash.  I could very easily fix it to not
crash, but I think that using KSTK_ESP on a remote task is
questionable in general.  Therefore, I propose to get rid of the
major users for 4.9.

This series makes two ABI changes:

 - /proc/PID/stat will show 0 0 instead of esp eip.  I don't think
   that the esp and eip fields were ever reliable unless the target
   task was being ptraced by the reading task, and ptrace(2) gives a
   far better interface to the same thing in this case.  On the flip
   side, these fields could leak kernel addresses under some
   circumstances on some arches if the target task is running or was
   interrupted (on a remote CPU or preempted on the local CPU) in an
   inconvenient place.

   I suspect it made sense when everything was single-CPU and non-
   preemptible, which implied that the target task *had* to be
   sleeping in something resembling normal kernel code, but that
   hasn't been the case for a long time.

 - /proc/PID/task/TID/maps did some interesting things to guess which
   vma was the stack.  This behavior is recent and IMO dangerously
   racy.  I'd like to get rid of it.

This is a little late so, if there is significant objection, I'll
just do the easy fix for 4.9 and resubmit for 4.10.

Andy Lutomirski (3):
  proc: Stop reporting eip and esp in /proc/PID/stat
  proc: Stop trying to report thread stacks
  mm: Change vm_is_stack_for_task() to vm_is_stack_for_current()

 Documentation/filesystems/proc.txt | 26 --------------------------
 fs/proc/array.c                    |  9 +++++----
 fs/proc/task_mmu.c                 | 29 ++++++++++-------------------
 fs/proc/task_nommu.c               | 28 ++++++++++------------------
 include/linux/mm.h                 |  2 +-
 mm/util.c                          |  4 +++-
 security/selinux/hooks.c           |  2 +-
 7 files changed, 30 insertions(+), 70 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1494227 — [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat

Reporting these fields on a non-current task is dangerous.  If the
task is in any state other than normal kernel code, they may contain
garbage or even kernel addresses on some architectures.  (x86_64
used to do this.  I bet lots of architectures still do.)  With
CONFIG_THREAD_INFO_IN_TASK, it can OOPS, too.

As far as I know, there are no use programs that make any material
use of these fields, so just get rid of them.

Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Cc: Kees Cook <keescook@chromium.org>
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/proc/array.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 88c7de12197b..1bb1097e73b7 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -417,10 +417,11 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
-		if (permitted) {
-			eip = KSTK_EIP(task);
-			esp = KSTK_ESP(task);
-		}
+		/*
+		 * esp and eip are intentionally zeroed out.  There is no
+		 * non-racy way to read them without freezing the task.
+		 * Programs that need reliable values can use ptrace(2).
+		 */
 	}
 
 	get_task_comm(tcomm, task);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1494237 — Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 30, 2016 at 10:58:56AM -0700, Andy Lutomirski wrote:
> Reporting these fields on a non-current task is dangerous.  If the
> task is in any state other than normal kernel code, they may contain
> garbage or even kernel addresses on some architectures.

Stupid question: Doesn't something similar apply to wchan?
Am I missing something?

It looks to me as if the get_wchan() implementation of X86 has the same
issue, with the difference that it's not obviously usable as an infoleak
because only known symbol names are printed, not numeric values.

get_wchan() basically does the following:

 - make sure the remote thread is sleeping (but don't take any locks to
   ensure it stays that way)
 - read the remote thread's saved kernel stack pointer
 - checks that the saved kernel stack pointer points to the main part of
   the remote stack
 - read a frame pointer through the saved kernel stack pointer; the read
   value could be anything if a race occured
 - check that the frame pointer points into the main part of the remote
   stack
 - read a saved instruction pointer through the frame pointer; the read
   value could be anything if a race occured
 - ensure that the "saved instruction pointer" doesn't point to the
   sections .sched.text and .spinlock.text
 - return the "saved instruction pointer"

So as far as I can tell, it *might* be possible for userspace to leak
information about the kernel text by alternatingly storing a proper
saved instruction pointer and a user-supplied value in a place from
which the kernel tries to read a saved instruction pointer.
By supplying values that are suspected to point to kernel text and
looking at the reported wchan values, an attacker could learn information
that allows him to e.g. defeat kASLR and even slowly locate functions in
custom kernels.

I vaguely remember seeing some similar code that defends against issues
like this by reading some counter beforehand and afterwards and checking
that it stays the same, but I'm not sure where that was. I think I saw it
while looking at the series for vmalloc()ed stacks.

[toc] | [prev] | [next] | [standalone]

#1494337 — Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat

[cc: PeterZ]

On Fri, Sep 30, 2016 at 11:56 AM, Jann Horn <jann@thejh.net> wrote:
> On Fri, Sep 30, 2016 at 10:58:56AM -0700, Andy Lutomirski wrote:
>> Reporting these fields on a non-current task is dangerous.  If the
>> task is in any state other than normal kernel code, they may contain
>> garbage or even kernel addresses on some architectures.
>
> Stupid question: Doesn't something similar apply to wchan?
> Am I missing something?
>
> It looks to me as if the get_wchan() implementation of X86 has the same
> issue, with the difference that it's not obviously usable as an infoleak
> because only known symbol names are printed, not numeric values.
>
> get_wchan() basically does the following:
>
>  - make sure the remote thread is sleeping (but don't take any locks to
>    ensure it stays that way)

Peter, how nasty would it be to add some lightish-weight lock that
lets us pin a task in a non-running state?  Maybe we could take the rq
lock, do something to the task to make it sleepy (steal it off the
queue?), unlock the lock, do whatever we're going, then take the lock
again and put it back.

Or if we had a seqlock-like thing, we could maybe arrange for
get_wchan to abort if the task get scheduled between when it starts
and when it finishes.

On an unrelated note, can we please lock down all the silly historical
*userspace* info leaks in /proc?  Nasty ones include: net, cmdline (at
the very least, only argv[0] should be visible if the reader lacks
ptrace access).

Less nasty ones include: limits, sched, autogroup, comm, wchan,
schedstat, cpuset, cgroup, oom_*, sessionid, coredump_filter

uid_map, gid_map, etc are just screwed up.  They should be per
*namespace* somewhere, and they should require creds on the namespace.

timerslack is totally fscked up -- it allows ugo to write and it
checks the wrong creds.  Jann, does your series fix that?

--Andy

[toc] | [prev] | [next] | [standalone]

#1494353 — Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat

On Fri, Sep 30, 2016 at 7:01 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Peter, how nasty would it be to add some lightish-weight lock that
> lets us pin a task in a non-running state?  Maybe we could take the rq
> lock, do something to the task to make it sleepy (steal it off the
> queue?), unlock the lock, do whatever we're going, then take the lock
> again and put it back.

No. Don't do this. Forcing some sleeping lock in the core task state
/proc stuff is a nightmare. That thing ends up being used very heavily
under some loads. No _way_ is it ok to synchronize with the target
task.

> Or if we had a seqlock-like thing, we could maybe arrange for
> get_wchan to abort if the task get scheduled between when it starts
> and when it finishes.

seq_lock might be ok, but do we even need it? What's the worst that
can happen? An odd symbol name showing up in a race condition? Sounds
like a non-issue to me.

        Linus

[toc] | [prev] | [next] | [standalone]

#1494390 — Re: [PATCH 1/3] proc: Stop reporting eip and esp in /proc/PID/stat

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 30, 2016 at 07:01:13PM -0700, Andy Lutomirski wrote:
> On an unrelated note, can we please lock down all the silly historical
> *userspace* info leaks in /proc?  Nasty ones include: net, cmdline (at
> the very least, only argv[0] should be visible if the reader lacks
> ptrace access).
> 
> Less nasty ones include: limits, sched, autogroup, comm, wchan,
> schedstat, cpuset, cgroup, oom_*, sessionid, coredump_filter

If that doesn't break stuff, I'm very much in favor of it.


> uid_map, gid_map, etc are just screwed up.  They should be per
> *namespace* somewhere, and they should require creds on the namespace.

What do you have in mind? Something like
/proc/namespaces/user:123456/{uid_map,gid_map,setgroups,parent_ns},
with jumped fake symlinks to the directory and its entries in /proc/$pid/?


> timerslack is totally fscked up -- it allows ugo to write and it
> checks the wrong creds.  Jann, does your series fix that?

Nope. Never noticed that thing so far, probably because it was only
added a few months ago. :/ Will add it to my series.

[toc] | [prev] | [next] | [standalone]

#1495091

On Fri, Sep 30, 2016 at 10:58 AM, Andy Lutomirski <luto@kernel.org> wrote:
> Jann Horn noticed that KSTK_ESP + eager task stack freeing was a bad
> combination and could crash.  I could very easily fix it to not
> crash, but I think that using KSTK_ESP on a remote task is
> questionable in general.  Therefore, I propose to get rid of the
> major users for 4.9.

Ping!

We need to decide fairly soon whether to apply these (or perhaps just
patch 1 or just patches 2 and 3) for 4.9.  For any parts that aren't
applied, I'll send quick fixups to pin the stack in the offending
code.

--Andy

[toc] | [prev] | [next] | [standalone]

#1495098

On Mon, Oct 3, 2016 at 4:08 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Ping!
>
> We need to decide fairly soon whether to apply these (or perhaps just
> patch 1 or just patches 2 and 3) for 4.9.  For any parts that aren't
> applied, I'll send quick fixups to pin the stack in the offending
> code.

I think we should apply it. Hopefully nothing uses it, and nobody will
notice. And if somebody *does* notice, the sooner we find out, the
better.

             Linus

[toc] | [prev] | [next] | [standalone]

#1495233

My personal opinion is that even looking at esp/rsp is asking for 
trouble.  The only reliable information is VM_STACK or another VM flag 
that makes the area expand in response to stack growth.

Besides, userspace could always play funky trampoline games with the 
stack pointer, or even dynamically expand the stack by doing a malloc 
if a stack overflow draws near, which would put the stack in the data 
section temporarily.

As long as esp is in the bounds of a valid VMA, my vote is that we 
should consider it undefined how the task uses it.

On Mon, Oct 3, 2016 at 4:17 PM, Linus Torvalds 
<torvalds@linux-foundation.org> wrote:
> On Mon, Oct 3, 2016 at 4:08 PM, Andy Lutomirski <luto@amacapital.net> 
> wrote:
>> 
>>  Ping!
>> 
>>  We need to decide fairly soon whether to apply these (or perhaps 
>> just
>>  patch 1 or just patches 2 and 3) for 4.9.  For any parts that aren't
>>  applied, I'll send quick fixups to pin the stack in the offending
>>  code.
> 
> I think we should apply it. Hopefully nothing uses it, and nobody will
> notice. And if somebody *does* notice, the sooner we find out, the
> better.
> 
>              Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web