linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jann Horn <jannh@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
	Vito Caputo <vcaputo@pengaru.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	x86@kernel.org, Qi Zheng <zhengqi.arch@bytedance.com>,
	peterz@infradead.org, luto@kernel.org
Subject: Re: CONFIG_ORC_UNWINDER=y breaks get_wchan()?
Date: Fri, 24 Sep 2021 01:59:54 +0200	[thread overview]
Message-ID: <CAG48ez2zC=+PuNgezH53HBPZ8CXU5H=vkWx7nJs60G8RXt3w0Q@mail.gmail.com> (raw)
In-Reply-To: <202109211931.E6E33500E8@keescook>

On Wed, Sep 22, 2021 at 5:05 AM Kees Cook <keescook@chromium.org> wrote:
> On Tue, Sep 21, 2021 at 05:15:37PM -0700, Josh Poimboeuf wrote:
> > On Tue, Sep 21, 2021 at 12:32:49PM -0700, Vito Caputo wrote:
> > > Is this an oversight of the ORC_UNWINDER implementation?  It's
> > > arguably a regression to completely break wchans for tools like `ps -o
> > > wchan` and `top`, or my window manager and its separate monitoring
> > > utility.  Presumably there are other tools out there sampling wchans
> > > for monitoring as well, there's also an internal use of get_chan() in
> > > kernel/sched/fair.c for sleep profiling.
> > >
> > > I've occasionally seen when monitoring at a high sample rate (60hz) on
> > > something churny like a parallel kernel or systemd build, there's a
> > > spurious non-zero sample coming out of /proc/[pid]/wchan containing a
> > > hexadecimal address like 0xffffa9ebc181bcf8.  This all smells broken,
> > > is get_wchan() occasionally spitting out random junk here kallsyms
> > > can't resolve, because get_chan() is completely ignorant of
> > > ORC_UNWINDER's effects?
> >
> > Hi Vito,
> >
> > Thanks for reporting this.  Does this patch fix your issue?
> >
> >   https://lkml.kernel.org/r/20210831083625.59554-1-zhengqi.arch@bytedance.com
> >
> > Though, considering wchan has been silently broken for four years, I do
> > wonder what the impact would be if we were to just continue to show "0"
> > (and change frame pointers to do the same).
> >
> > The kernel is much more cautious than it used to be about exposing this
> > type of thing.  Can you elaborate on your use case?
> >
> > If we do keep it, we might want to require CAP_SYS_ADMIN anyway, for
> > similar reasons as
> >
> >   f8a00cef1720 ("proc: restrict kernel stack dumps to root")
>
> Normally wchan is protected by:
>
>         ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)
>
> I might argue that this check isn't right -- it needs to be using
> f_cred, but I'll let Jann answer more there.
>
> > ... since presumably proc_pid_wchan()'s use of '%ps' can result in an
> > actual address getting printed if the unwind gets confused, thanks to
> > __sprint_symbol()'s backup option if kallsyms_lookup_buildid() doesn't
> > find a name.
>
> Ew, yeah, __sprint_symbol() falls back to exposing addresses. :(
>
>         name = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
>                                        buffer);
>         if (!name)
>                 return sprintf(buffer, "0x%lx", address - symbol_offset);

Whaaaat? That's not how wchan worked when I looked at this the last
time a few years ago...

... ah, commit 152c432b128cb ("proc/wchan: use printk format instead
of lookup_symbol_name()") is to blame for that.

> Thought I can't immediately think of what wouldn't be symbolized by
> kallsyms_lookup_buildid(), but given it fails open, I can totally
> believe there is. :)

One thing that makes /proc/$pid/wchan and /proc/$pid/stack so awful is
that they unwind a task's stack without ensuring that that stack isn't
actually running. That's why /proc/$pid/stack got restricted to root
years ago, see <https://bugs.chromium.org/p/project-zero/issues/detail?id=1657>
- I wrote an exploit back then that manages to read a pointer from an
arbitrary stack location via /proc/$pid/stack.

>         is_ksym_addr()
>         module_address_lookup()
>         bpf_address_lookup()
>         ftrace_mod_address_lookup()
>
> > Though, instead of requiring CAP_SYS_ADMIN, maybe we can just fix
> > __sprint_symbol() to not expose addresses?
> >
> > Or is there some other reason for needing CAP_SYS_ADMIN?  Jann?
>
> While it's not very high fidelity, I don't like having the kernel
> symbols exposed like this because userspace can basically sample the
> execution path of syscalls, etc. It's not a raw value, but it still
> creeps me out given that it can be probed.
>
> So, if it's been broken for 4 years under ORC, how about we just disable
> wchan permanently? (Untested...)

If that's possible, I would like it very much. I don't know whether
there are people on arm64 or so that rely on it though...

  reply	other threads:[~2021-09-24  0:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 19:32 Vito Caputo
2021-09-22  0:15 ` Josh Poimboeuf
2021-09-22  3:04   ` Kees Cook
2021-09-23 23:59     ` Jann Horn [this message]
2021-09-25 19:07       ` David Laight
2021-09-22  3:30   ` Qi Zheng
2021-10-05  0:51     ` Andy Lutomirski
2021-09-24  5:46   ` Vito Caputo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAG48ez2zC=+PuNgezH53HBPZ8CXU5H=vkWx7nJs60G8RXt3w0Q@mail.gmail.com' \
    --to=jannh@google.com \
    --cc=jpoimboe@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=peterz@infradead.org \
    --cc=vcaputo@pengaru.com \
    --cc=x86@kernel.org \
    --cc=zhengqi.arch@bytedance.com \
    --subject='Re: CONFIG_ORC_UNWINDER=y breaks get_wchan()?' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).