From: Linus Torvalds <email@example.com> To: Eric Dumazet <firstname.lastname@example.org> Cc: Thomas Gleixner <email@example.com>, linux-kernel <firstname.lastname@example.org>, Eric Dumazet <email@example.com> 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: <CAHk-=wjbvzCAhAtvG0d81W5o0-KT5PPTHhfJ5ieDFq+bGtgOYg@mail.gmail.com> (raw) In-Reply-To: <firstname.lastname@example.org> On Fri, Apr 16, 2021 at 12:24 PM Eric Dumazet <email@example.com> wrote: > > From: Eric Dumazet <firstname.lastname@example.org> > > 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
next prev 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: 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' \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to_user()' \ /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
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).