linux-kernel - Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 24 Feb 2019 09:26:45 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        stable <stable@...r.kernel.org>,
        Changbin Du <changbin.du@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access
 kernel memory that can fault

On Sun, Feb 24, 2019 at 7:18 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> On Sat, 23 Feb 2019 20:38:03 -0800
> Andy Lutomirski <luto@...nel.org> wrote:
> >
> > Can we just get rid of this might_sleep()?  access_ok() doesn't sleep
> > as far as I know.
>
> Hmm, which might_sleep() would you pointed? What I talked was a
> WARN_ON_ONCE(!in_task()) in access_ok() on x86 (only!), and in_task() just
> checks preempt_count.

So the in_task() check does kind of make sense. Using "access_ok()"
outside of task context is certainly an odd thing, for several
reasons. The main one being simply that outside of task context, the
whole "which task" question is open, and you don't know if the task is
the active one, and so it's not clear if whatever task you interrupt
might have done "set_fs()" or not.

So PeterZ isn't wrong:

> I guess PeterZ assumed that access_ok() is used only with user space access
> APIs (e.g. copy_from_user) which can cause page-fault and locks mm (and might
> sleep :)), but now we are trying to use access_ok() with new functions which
> disables page-fault and just return -EFAULT.

.. but in this case, if we do it all *within* code that saves and
restores the user access flag with get_fs/set_fs, access_ok() would be
ok and it doesn't have the above issue.

So access_ok() in _general_ is absolutely not safe to do from
interrupts, but within the context of probing user memory from a
tracing event it just happens to be ok.

It would be lovely to have a special macro for this, and keep the
warning for the general case, but because this is a "every
architecture needs to build their own" it's probably too painful.

PeterZ, do you remember the particular use case that triggered that
commit 7c4788950ba5 ("x86/uaccess, sched/preempt: Verify access_ok()
context")?

                    Linus