From: Linus Torvalds <torvalds@linux-foundation.org> To: Masami Hiramatsu <mhiramat@kernel.org> Cc: kernel test robot <lkp@intel.com>, Steven Rostedt <rostedt@goodmis.org>, Shuah Khan <shuah@kernel.org>, Linux List Kernel Mailing <linux-kernel@vger.kernel.org>, Andy Lutomirski <luto@amacapital.net>, Ingo Molnar <mingo@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, Changbin Du <changbin.du@gmail.com>, Jann Horn <jannh@google.com>, Kees Cook <keescook@chromium.org>, Andy Lutomirski <luto@kernel.org>, Alexei Starovoitov <alexei.starovoitov@gmail.com>, Nadav Amit <namit@vmware.com>, Peter Zijlstra <peterz@infradead.org>, Joel Fernandes <joel@joelfernandes.org>, yhs@fb.com, lkp@01.org Subject: Re: [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x Date: Mon, 4 Mar 2019 10:59:22 -0800 [thread overview] Message-ID: <CAHk-=wiQM84VsAiw92UWyQZTQxr+F032Gm5BiFBXk7tC+LQohQ@mail.gmail.com> (raw) In-Reply-To: <20190304180610.2d4f6f08d9ad89d6abae3597@kernel.org> 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()). Your "unsafe" version for tracing that actually sets "set_fs(USER_DS)" is thje only thing that should use that helper. > > 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. > 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". 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". 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()". Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org> To: lkp@lists.01.org Subject: Re: [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x Date: Mon, 04 Mar 2019 10:59:22 -0800 [thread overview] Message-ID: <CAHk-=wiQM84VsAiw92UWyQZTQxr+F032Gm5BiFBXk7tC+LQohQ@mail.gmail.com> (raw) In-Reply-To: <20190304180610.2d4f6f08d9ad89d6abae3597@kernel.org> [-- Attachment #1: Type: text/plain, Size: 2036 bytes --] 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()). Your "unsafe" version for tracing that actually sets "set_fs(USER_DS)" is thje only thing that should use that helper. > > 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. > 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". 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". 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()". Linus
next prev parent reply other threads:[~2019-03-04 18:59 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-28 16:02 [PATCH v5 0/6] tracing/probes: uaccess: Add support user-space access Masami Hiramatsu 2019-02-28 16:02 ` [PATCH v5 1/6] uaccess: Add user_access_ok() Masami Hiramatsu 2019-02-28 16:03 ` [PATCH v5 2/6] uaccess: Use user_access_ok() in user_access_begin() Masami Hiramatsu 2019-03-03 17:39 ` [uaccess] 780464aed0: WARNING:at_arch/x86/include/asm/uaccess.h:#strnlen_user/0x kernel test robot 2019-03-03 17:39 ` kernel test robot 2019-03-03 19:53 ` Linus Torvalds 2019-03-03 19:53 ` Linus Torvalds 2019-03-04 1:14 ` Masami Hiramatsu 2019-03-04 1:14 ` Masami Hiramatsu 2019-03-04 2:37 ` Linus Torvalds 2019-03-04 2:37 ` Linus Torvalds 2019-03-04 9:06 ` Masami Hiramatsu 2019-03-04 9:06 ` Masami Hiramatsu 2019-03-04 15:16 ` Masami Hiramatsu 2019-03-04 15:16 ` Masami Hiramatsu 2019-03-04 15:58 ` Jann Horn 2019-03-04 15:58 ` Jann Horn 2019-03-04 18:59 ` Linus Torvalds [this message] 2019-03-04 18:59 ` Linus Torvalds 2019-03-05 2:36 ` Masami Hiramatsu 2019-03-05 2:36 ` Masami Hiramatsu 2019-03-05 8:22 ` Masami Hiramatsu 2019-03-05 8:22 ` Masami Hiramatsu 2019-03-05 9:01 ` Masami Hiramatsu 2019-03-05 9:01 ` Masami Hiramatsu 2019-03-05 9:07 ` Peter Zijlstra 2019-03-05 9:07 ` Peter Zijlstra 2019-03-05 13:58 ` Masami Hiramatsu 2019-03-05 13:58 ` Masami Hiramatsu 2019-03-05 14:53 ` Peter Zijlstra 2019-03-05 14:53 ` Peter Zijlstra 2019-03-05 15:18 ` Masami Hiramatsu 2019-03-05 15:18 ` Masami Hiramatsu 2019-03-04 3:20 ` [LKP] " Rong Chen 2019-03-04 3:20 ` Rong Chen 2019-02-28 16:03 ` [PATCH v5 3/6] uaccess: Add non-pagefault user-space read functions Masami Hiramatsu 2019-02-28 22:49 ` Yonghong Song 2019-03-01 2:29 ` Masami Hiramatsu 2019-03-01 6:30 ` Yonghong Song 2019-02-28 16:04 ` [PATCH v5 4/6] tracing/probe: Add ustring type for user-space string Masami Hiramatsu 2019-02-28 16:04 ` [PATCH v5 5/6] tracing/probe: Support user-space dereference Masami Hiramatsu 2019-02-28 16:05 ` [PATCH v5 6/6] selftests/ftrace: Add user-memory access syntax testcase Masami Hiramatsu
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='CAHk-=wiQM84VsAiw92UWyQZTQxr+F032Gm5BiFBXk7tC+LQohQ@mail.gmail.com' \ --to=torvalds@linux-foundation.org \ --cc=akpm@linux-foundation.org \ --cc=alexei.starovoitov@gmail.com \ --cc=changbin.du@gmail.com \ --cc=jannh@google.com \ --cc=joel@joelfernandes.org \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@01.org \ --cc=lkp@intel.com \ --cc=luto@amacapital.net \ --cc=luto@kernel.org \ --cc=mhiramat@kernel.org \ --cc=mingo@kernel.org \ --cc=namit@vmware.com \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ --cc=shuah@kernel.org \ --cc=yhs@fb.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.