From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Russell King - ARM Linux <linux@armlinux.org.uk>,
"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Richard Henderson <rth@twiddle.net>,
Will Deacon <will.deacon@arm.com>,
Haavard Skinnemoen <hskinnemoen@gmail.com>,
Vineet Gupta <vgupta@synopsys.com>,
Steven Miao <realmz6@gmail.com>,
Jesper Nilsson <jesper.nilsson@axis.com>,
Mark Salter <msalter@redhat.com>,
Yoshinori Sato <ysato@users.sourceforge.jp>,
Richard Kuo <rkuo@codeaurora.org>,
Tony Luck <tony.luck@intel.com>,
Geert Uytterhoeven <geert@linux-m68k.org>,
James Hogan <james.hogan@imgtec.com>,
Michal Simek <monstr@monstr.eu>,
David Howells <dhowells@redhat.com>,
Ley Foon Tan <lftan@altera.com>, Jonas Bonn <jonas@southpole.se>,
Helge Deller <deller@gmx.de>,
Martin Schwidefsky <schwidefsky@de.ibm.com>,
Ralf Baechle <ralf@linux-mips.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Chen Liqin <liqin.linux@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Chris Metcalf <cmetcalf@mellanox.com>,
Richard Weinberger <richard@nod.at>,
Guan Xuetao <gxt@mprc.pku.edu.cn>,
Thomas Gleixner <tglx@linutronix.de>,
Chris Zankel <chris@zankel.net>
Subject: Re: [RFC][CFT][PATCHSET v1] uaccess unification
Date: Thu, 30 Mar 2017 22:08:16 +0100 [thread overview]
Message-ID: <20170330210816.GV29622@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CA+55aFwktbgtL_x4gKqcJU6=FrkokneLRQ30HtDhuR2WErG83w@mail.gmail.com>
On Thu, Mar 30, 2017 at 12:19:35PM -0700, Linus Torvalds wrote:
> On Thu, Mar 30, 2017 at 12:10 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > That they very definitely should not. And not because of access_ok() or
> > might_fault() - this is one place where zero-padding is absolutely wrong.
> > So unless you are going to take it out of copy_from_user() and pray
> > that random shit ioctls in random shit drivers check the return value
> > properly, copy_from_user() is no-go here.
>
> Actually, that is a great example of why you should *not* use
> __copy_from_user().
>
> If the reason is lack of zero-padding, that doesn't mean that suddenly
> we shouldn't check the range. And it doesn't mean that it shouldn't
> document why it does it.
>
> So dammit, just add something like this to lib/iovec.c:
>
> static inline unsigned long copy_from_user_nozero(void *to, const
> void __user *from, size_t len)
> {
> if (!access_ok(from, len))
> return len;
> return __copy_from_user(to, from, len);
> }
>
> which now isn't insecure, and also magically documents *why* you don't
> just use the plain copy_from_user().
Maybe... However, we *do* have places where it's done under kmap_atomic()
in there. Let's leave that one until this round of uaccess consolidation is
finished, OK? lib/iov_iter.c is special and isolated enough; we can figure
out what to do with those primitives later.
As far as I'm concerned, lib/*.c and mm/*.c are separate story; I would start
with getting rid of that stuff in random drivers. Here's what we have at the
moment:
there are only 3 irregular callers of __copy_to_user_inatomic():
arch/mips/kernel/unaligned.c:1276: res = __copy_to_user_inatomic(addr, fpr, sizeof(*fpr));
drivers/gpu/drm/i915/i915_gem.c:913: ret = __copy_to_user_inatomic(user_data, vaddr + offset, length);
drivers/gpu/drm/i915/i915_gem.c:983: unwritten = __copy_to_user_inatomic(user_data, vaddr + offset, length);
There are 32 irregular callers of __copy_from_user_inatomic(), majority in
perf/oprofile-related code. Leave those aside, only 8 are left:
arch/mips/kernel/unaligned.c:1242: res = __copy_from_user_inatomic(fpr, addr,
drivers/gpu/drm/i915/i915_gem.c:1324: ret = __copy_from_user_inatomic(vaddr + offset, user_data, len);
drivers/gpu/drm/i915/i915_gem_execbuffer.c:669: unwritten = __copy_from_user_inatomic(r, user_relocs, count*sizeo
f(r[0]));
drivers/gpu/drm/msm/msm_gem_submit.c:73: return __copy_from_user_inatomic(to, from, n);
kernel/trace/trace.c:5780: len = __copy_from_user_inatomic(&entry->buf, ubuf, cnt);
kernel/trace/trace.c:5851: len = __copy_from_user_inatomic(&entry->id, ubuf, cnt);
kernel/trace/trace_kprobe.c:216: ret = __copy_from_user_inatomic(&c, (u8 *)addr + len, 1);
virt/kvm/kvm_main.c:1832: r = __copy_from_user_inatomic(data, (void __user *)addr + offset, len);
Ones in perf and oprofile code really smell like a missing helper,
along the lines of probe_kernel_read(), but for userland pointers.
Incidentally, metag, mips, openrisc and xtensa instances of that lack
pagefault_disable() - might be a bug, need to check that. powerpc and
sparc ones also lack it, but those have pagefault_disable() done in
caller. tile ones open-code access_ok(), AFAICS. Sorting that pile
out would already about half the amount of callers.
Ho-hum... There's something odd about those - some of them seem to
assume that we are under set_fs(USER_DS), some do what access_ok()
would've done with USER_DS and proceed to __copy_from_user_inatomic().
And that includes the ones like sparc... Very strange.
Am I right assuming that perf_callchain_user() can't be called other than
with USER_DS, but oprofile ->backtrace() can? I'm not familiar enough
with oprofile guts... Folks?
next prev parent reply other threads:[~2017-03-30 21:08 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-29 5:57 [RFC][CFT][PATCHSET v1] uaccess unification Al Viro
2017-03-29 20:08 ` Vineet Gupta
2017-03-29 20:29 ` Al Viro
2017-03-29 20:37 ` Linus Torvalds
2017-03-29 21:03 ` Al Viro
2017-03-29 21:24 ` Linus Torvalds
2017-03-29 23:09 ` Al Viro
2017-03-29 23:43 ` Linus Torvalds
2017-03-30 15:31 ` Al Viro
2017-03-29 21:14 ` Vineet Gupta
2017-03-29 23:42 ` Al Viro
2017-03-30 0:02 ` Vineet Gupta
2017-03-30 0:27 ` Linus Torvalds
2017-03-30 1:15 ` Al Viro
2017-03-30 20:40 ` Vineet Gupta
2017-03-30 20:59 ` Linus Torvalds
2017-03-30 23:21 ` Russell King - ARM Linux
2017-03-30 12:32 ` Martin Schwidefsky
2017-03-30 14:48 ` Al Viro
2017-03-30 16:22 ` Russell King - ARM Linux
2017-03-30 16:43 ` Al Viro
2017-03-30 17:18 ` Linus Torvalds
2017-03-30 18:48 ` Al Viro
2017-03-30 18:54 ` Al Viro
2017-03-30 18:59 ` Linus Torvalds
2017-03-30 19:10 ` Al Viro
2017-03-30 19:19 ` Linus Torvalds
2017-03-30 21:08 ` Al Viro [this message]
2017-03-30 18:56 ` Linus Torvalds
2017-03-31 0:21 ` Kees Cook
2017-03-31 13:38 ` James Hogan
2017-04-03 16:27 ` James Morse
2017-04-04 20:26 ` Max Filippov
2017-04-04 20:52 ` Al Viro
2017-04-05 5:05 ` ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) Al Viro
2017-04-05 8:08 ` Al Viro
2017-04-05 18:44 ` Tony Luck
2017-04-05 20:33 ` Al Viro
2017-04-07 0:24 ` [RFC][CFT][PATCHSET v2] uaccess unification Al Viro
2017-04-07 0:35 ` Al Viro
[not found] <CACVxJT8+fQqvpSPb9rTWFy6g7moqUqxi+Ewjcg0ykuqo=vm4Ow@mail.gmail.com>
2017-03-30 13:27 ` [RFC][CFT][PATCHSET v1] " Alexey Dobriyan
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=20170330210816.GV29622@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=benh@kernel.crashing.org \
--cc=chris@zankel.net \
--cc=cmetcalf@mellanox.com \
--cc=davem@davemloft.net \
--cc=deller@gmx.de \
--cc=dhowells@redhat.com \
--cc=geert@linux-m68k.org \
--cc=gxt@mprc.pku.edu.cn \
--cc=hskinnemoen@gmail.com \
--cc=james.hogan@imgtec.com \
--cc=jesper.nilsson@axis.com \
--cc=jonas@southpole.se \
--cc=lftan@altera.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=liqin.linux@gmail.com \
--cc=monstr@monstr.eu \
--cc=msalter@redhat.com \
--cc=ralf@linux-mips.org \
--cc=realmz6@gmail.com \
--cc=richard@nod.at \
--cc=rkuo@codeaurora.org \
--cc=rth@twiddle.net \
--cc=schwidefsky@de.ibm.com \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=torvalds@linux-foundation.org \
--cc=vgupta@synopsys.com \
--cc=will.deacon@arm.com \
--cc=ysato@users.sourceforge.jp \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).