linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: [RFC] memdup_user() and friends
Date: Sun, 7 Jan 2018 02:16:56 +0000	[thread overview]
Message-ID: <20180107021651.GB13338@ZenIV.linux.org.uk> (raw)

	After reviewing memdup_user() callers, I've found several places
where it got completely unbounded values passed for size (up to 2Gb),
as well as some bounded by ridiculously high values - e.g.
        if (size > 1024 * 128)  /* sane value */
                return -EINVAL;

        container = memdup_user(buf, size);
        if (IS_ERR(container))
                return PTR_ERR(container);
in sound/core/control.c:replace_user_tlv().  To add insult to injury, that
particular caller uses the result only for memcmp() and copy_to_user(), so
there's no point whatsoever to try and keep the copy physically contiguous.
IOW, it would be just fine with vmalloc()-based analogue of memdup_user().

	There is a bunch of places where a kvmalloc-based analogue is
open-coded and they deserve a primitive of their own.  We can't convert
memdup_user() itself to that - it would have to be paired with kvfree()
instead of kfree() and we *do* have callers that need kmalloc() - e.g.
when the copy is passed to usb_bulk_msg(), etc.  So it needs to be
a separate primitive.

However, memdup_user() itself has another fun issue:
        /*
         * Always use GFP_KERNEL, since copy_from_user() can sleep and
         * cause pagefault, which makes it pointless to use GFP_NOFS
         * or GFP_ATOMIC.
         */
        p = kmalloc_track_caller(len, GFP_KERNEL);
        if (!p)
                return ERR_PTR(-ENOMEM);
Sure, GFP_ATOMIC and GFP_NOFS would be pointless.  However, GFP_USER and,
possibly, __GFP_NOWARN would not.  As the matter of fact, of 200-odd callers
only for two I couldn't prove that they'd be fine with GFP_USER:
arch/x86/kvm/x86.c:2076:        page = memdup_user(blob_addr + (page_num * PAGE_SIZE), PAGE_SIZE);
include/rdma/ib_verbs.h:2437:   buf = memdup_user(p, len);

The former is weird xen crap used by one place in kvm_set_msr_common() and
I've given up trying to sort the call chains out; it might very well be fine
with GFP_USER, actually.  The latter is a flaming atrocity from the
bowels of infiniband.  Might or might not be fine with GFP_USER, but in
any case that code is utter crap.  What it tries to do is verifying that given
range of userland memory contains only zeroes.  Callers are also not pretty and
hard to track, as well.

Everything else is definitely fine with GFP_USER - it's stuff like "copy of ioctl
arguments in an ioctl never issued by the kernel code, must have come straight from
ioctl(2)" and things like that.  IMO we should simply switch memdup_user() to
GFP_USER and be done with that.  Limiting the size ought to be done by callers and
IMO there's no point in __GFP_NOWARN there.

What I propose is
	* switch memdup_user() to GFP_USER
	* add vmemdup_user(), using kvmalloc() instead of kmalloc() (also with
GFP_USER)
	* switch open-coded instances of the latter to calling it
	* switch some of the memdup_user() callers to vmemdup_user() - the ones that
don't need physically contiguous copy and might be larger than a couple of pages.
	* add apriori bounds on size in the call sites that do not have those yet -
that'll require comments from maintainers of the code in question in some cases.

Objections?

             reply	other threads:[~2018-01-07  2:17 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-07  2:16 Al Viro [this message]
2018-01-08 14:57 ` [RFC] memdup_user() and friends Marcelo Ricardo Leitner
2018-01-08 17:26 ` Andy Shevchenko

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=20180107021651.GB13338@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /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).