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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHk-=whNf_n1WXWW+ugAVeL5ZK0GcEP3cTYocju1nS85VtMjjQ@mail.gmail.com>
Date:   Fri, 22 Feb 2019 09:43:14 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Andy Lutomirski <luto@...capital.net>,
        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>,
        Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access
 kernel memory that can fault

On Fri, Feb 22, 2019 at 12:35 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> Or, can we do this?
>
> long __probe_user_read(void *dst, const void *src, size_t size)
> {

Add a

        if (!access_ok(src, size))
                ret = -EFAULT;
        else {
.. do the pagefault_disable() etc ..
        }

to after the "set_fs()", and it looks good to me. Make it clear that
yes, this works _only_ for user reads.

Adn that makes all the games with "kernel_uaccess_faults_ok"
pointless, so you can just remove them.

(note that the "access_ok()" has to come after we've done "set_fs()",
because it takes the address limit from that).

Also, since normally we'd expect that we already have USER_DS, it
might be worthwhile to do this with a wrapper, something along the
lines of

        mm_segment_t old_fs = get_fs();

        if (segment_eq(old_fs, USER_DS))
              return __normal_probe_user_read();
        set_fs(USER_DS);
        ret = __normal_probe_user_read();
        set_fs(old_fs);
        return ret;

and have that __normal_probe_user_read() just do

        if (!access_ok(src, size))
                return -EFAULT;
        pagefault_disable();
        ret = __copy_from_user_inatomic(dst, ...);
        pagefault_enable();
        return ret ? -EFAULT : 0;

which looks more obvious.

Also, I would suggest that you just make the argument type be "const
void __user *", since the whole point is that this takes a user
pointer, and nothing else.

Then we should still probably fix up "__probe_kernel_read()" to not
allow user accesses. The easiest way to do that is actually likely to
use the "unsafe_get_user()" functions *without* doing a
uaccess_begin(), which will mean that modern CPU's will simply fault
on a kernel access to user space.

The nice thing about that is that usually developers will have access
to exactly those modern boxes, so the people who notice that it
doesn't work are the right people.

Alternatively, we should just make it be architecture-specific, so
that architectures can decide "this address cannot be a kernel
address" and refuse to do it.

                Linus