Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210416192413.1514419-1-eric.dumazet@gmail.com> <CAHk-=wjbvzCAhAtvG0d81W5o0-KT5PPTHhfJ5ieDFq+bGtgOYg@mail.gmail.com>
In-Reply-To: <CAHk-=wjbvzCAhAtvG0d81W5o0-KT5PPTHhfJ5ieDFq+bGtgOYg@mail.gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Sat, 17 Apr 2021 21:44:09 +0200
Message-ID: <CANn89iK0Win0m5ggB-EjFvVwmpkyg_nG9FW9uzREmrpoeTF_aw@mail.gmail.com>
Subject: Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to_user()
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Eric Dumazet <eric.dumazet@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Apr 17, 2021 at 6:03 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

I thought put_cmsg() callers were from the kernel, with no possibility
for user to abuse this interface trying to push GB of data.

Which would be terrible even if we  ' fix'  possible overflows.

Maybe I was wrong.


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