archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <>
To: Eric Dumazet <>
Cc: Thomas Gleixner <>,
	linux-kernel <>,
	Eric Dumazet <>
Subject: Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to_user()
Date: Sat, 17 Apr 2021 09:03:02 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Apr 16, 2021 at 12:24 PM Eric Dumazet <> wrote:
> From: Eric Dumazet <>
> 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.


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

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.


  parent reply	other threads:[~2021-04-17 16:03 UTC|newest]

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \
    --subject='Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to_user()' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).