On Sat, Apr 17, 2021 at 6:03 PM Linus Torvalds <torva...@linux-foundation.org> wrote: > > On Fri, Apr 16, 2021 at 12:24 PM Eric Dumazet <eric.duma...@gmail.com> wrote: > > > > From: Eric Dumazet <eduma...@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
