lkml.org 
[lkml]   [2019]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x
    On Mon, 4 Mar 2019 10:59:22 -0800
    Linus Torvalds <torvalds@linux-foundation.org> wrote:

    > On Mon, Mar 4, 2019 at 1:06 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
    > >
    > > On Sun, 3 Mar 2019 18:37:59 -0800
    > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
    > > > We've had this before. We've gotten rid of the actual "use system
    > > > calls", but we still have some of the init sequence in particular just
    > > > calling the wrappers instead.
    > >
    > > Are those safe if we are in init sequence?
    >
    > Yes, they are, it runs with set_fs(KERNEL_DS).
    >
    > But the patches made that now complain about copying from non-user
    > space, even though it's fine.
    >
    > Basically, "strncpy_from_user()" shouldn't use "user_access_ok()",
    > since it actually can take a kernel address (with set_fs()).

    OK, so strncpy_from_user() or any other copy_from_user() should be
    available for copying kernel memory if set_fs(KERNEL_DS).

    > Your "unsafe" version for tracing that actually sets "set_fs(USER_DS)"
    > is thje only thing that should use that helper.

    I see, it ensures it is accessing user-memory.

    >
    > > > And yes, ksys_mount() takes __user pointers.
    > > >
    > > > It would be a lot better to use "do_mount()", which is the interface
    > > > that takes actual "char *" pointers.
    > >
    > > Unfortunately, it still takes a __user pointer.
    >
    > Ahh, yes, the name remains in user space.
    >
    > Besides, I'm sure you'd just hit other cases instead where people use
    > set_fs() and copy strings.

    Yeah, under init/ I saw such cases.

    >
    > > So what we need is
    > >
    > > long do_mount(const char *dev_name, struct path *dir_path,
    > > const char *type_page, unsigned long flags, void *data_page)
    > >
    > > or introduce kern_do_mount()?
    >
    > It's actually fairly painful. Particularly because of that "void *data_page".

    Yeah, that is what I've hit while testing :-(

    >
    > Your second email with "Would this work?" helper function _wopuldn't_
    > work correctly, exactly because you passed in a regular string to the
    > data page.
    >
    > Also, I don't want to see code that replaces the unconditional "copy
    > path from user space" with a conditional "do we have path in kernel
    > space".

    Yes, that's just a hack :)

    >
    > So together with the whole "uyou'll hit other peoblems anyway", I
    > don't think this is a good approach.
    >
    > I think you simply need to have a separate "unsafe_strncpy()"
    > function, and not change the existing "strncpy_from_user()".

    Would you mean implementing yet another "strncpy_from_user without
    pagefault"?
    What we changed here is just use user_access_ok() instead access_ok()
    in user_access_begin() because access_ok() may cause false-positive
    warning if we use it in IRQ.

    I think the better way to do this is allowing strncpy_from_user()
    if some conditions are match, like

    - strncpy_from_user() will be able to copy user memory with set_fs(USER_DS)
    - strncpy_from_user() can copy kernel memory with set_fs(KERNEL_DS)
    - strncpy_from_user() can access unsafe memory in IRQ context if
    pagefault is disabled.

    This is almost done, except for CONFIG_DEBUG_ATOMIC_SLEEP=y on x86.

    So, what about adding a condition to WARN_ON_IN_IRQ() like below
    instead of introducing user_access_ok() ?

    diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
    index 780f2b42c8ef..ec0f0b74c9ab 100644
    --- a/arch/x86/include/asm/uaccess.h
    +++ b/arch/x86/include/asm/uaccess.h
    @@ -70,7 +70,7 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
    })

    #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
    -# define WARN_ON_IN_IRQ() WARN_ON_ONCE(!in_task())
    +# define WARN_ON_IN_IRQ() WARN_ON_ONCE(pagefault_disabled() && !in_task())
    #else
    # define WARN_ON_IN_IRQ()
    #endif
    Of course we have to move pagefault_disabled() macro in somewhere better place.

    Thank you,

    --
    Masami Hiramatsu <mhiramat@kernel.org>

    \
     
     \ /
      Last update: 2019-03-05 03:37    [W:3.283 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site