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]

Message-Id: <AD9148E4-00CC-4711-82D3-957D28497057@amacapital.net>
Date:   Fri, 22 Feb 2019 09:48:21 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Masami Hiramatsu <mhiramat@...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>,
        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 Feb 22, 2019, at 9:43 AM, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
>> 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.

We use probe_kernel_read() from oops code. I’d rather it return -EFAULT than oops harder and kill the first oops.