From: Jann Horn <email@example.com> To: Kees Cook <firstname.lastname@example.org> Cc: Josh Poimboeuf <email@example.com>, Vito Caputo <firstname.lastname@example.org>, linux-kernel <email@example.com>, firstname.lastname@example.org, Qi Zheng <email@example.com>, firstname.lastname@example.org, email@example.com 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 <firstname.lastname@example.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://email@example.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...
next prev parent 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' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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).