LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Eric Dumazet <edumazet@google.com>
Subject: Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to_user()
Date: Sat, 17 Apr 2021 09:03:02 -0700
Message-ID: <CAHk-=wjbvzCAhAtvG0d81W5o0-KT5PPTHhfJ5ieDFq+bGtgOYg@mail.gmail.com> (raw)
In-Reply-To: <20210416192413.1514419-1-eric.dumazet@gmail.com>

On Fri, Apr 16, 2021 at 12:24 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
>
> We have to loop only to copy u64 values.
> After this first loop, we copy at most one u32, one u16 and one byte.

As Al mentioned, at least in trivial cases the compiler understands
that the subsequent loops can only be executed once, because earlier
loops made sure that 'len' is always smaller than 2*size.

But apparently the problem is the slightly more complex cases where
the compiler just messes up and loses sight of that. Oh well.

So the patch looks fine to me.

HOWEVER.

Looking at the put_cmsg() case in net-next, I'm very *VERY* unhappy
about how you use those "unsafe" user access functions.

Why? Because the point of the "unsafe" is to be a big red flag and
make sure you are very VERY careful with it.

And that code isn't.

In particular, what if the "int len" argument is negative? Maybe it
cannot happen, but when it comes to things like those unsafe user
accessors, I really really want to see that all the arguments are
*checked*.

And you don't.

You do

        if (!user_write_access_begin(cm, cmlen))

ahead of time, and that will do basic range checking, but "cmlen" is

        sizeof(struct cmsghdr) + (len))

so it's entirely possible that "cmlen" has a valid value, but "len"
(and thus "cmlen - sizeof(*cm)", which is the length argument to the
unsafe user copy) might be negative and that is never checked.

End result: I want people to be a LOT more careful when they use those
unsafe user space accessors. You need to make it REALLY REALLY obvious
that everything you do is safe. And it's not at all obvious in the
context of put_cmsg() - the safety currently relies on every single
caller getting it right.

So either fix "len" to be some restricted type (ie "unsigned short"),
or make really really sure that "len" is valid (ie never negative, and
the cmlen addition didn't overflow.

Really. The "unsafe" user accesses are named that way very explicitly,
and for a very very good reason: the safety needs to be guaranteed and
obvious within the context of those accesses. Not within some "oh,
nobody will ever call this with a negative argument" garbage bullshit.

                  Linus

  parent reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-16 19:24 Eric Dumazet
2021-04-16 19:44 ` Al Viro
2021-04-16 20:11   ` Eric Dumazet
2021-04-16 20:57     ` Eric Dumazet
2021-04-17 13:59   ` David Laight
2021-04-17 16:03 ` Linus Torvalds [this message]
2021-04-17 16:08   ` Linus Torvalds
2021-04-17 16:27     ` Linus Torvalds
2021-04-17 18:09       ` Al Viro
2021-04-17 20:30         ` Al Viro
2021-04-17 20:35           ` Al Viro
2021-04-17 22:11             ` Linus Torvalds
2021-04-18  0:50               ` Al Viro
2021-04-17 19:44   ` Eric Dumazet
2021-04-17 19:51     ` Linus Torvalds

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-=wjbvzCAhAtvG0d81W5o0-KT5PPTHhfJ5ieDFq+bGtgOYg@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git