Messages in this thread | | | From | Linus Torvalds <> | Date | Thu, 30 Mar 2017 10:18:23 -0700 | Subject | Re: [RFC][CFT][PATCHSET v1] uaccess unification |
| |
On Thu, Mar 30, 2017 at 9:43 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Thu, Mar 30, 2017 at 05:22:41PM +0100, Russell King - ARM Linux wrote: > How would the following affect things? > > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index e68604ae3ced..d24d338f0682 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c > @@ -184,7 +184,7 @@ static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t b > > kaddr = kmap(page); > from = kaddr + offset; > - left = __copy_to_user(buf, from, copy); > + left = __copy_to_user_inatomic(buf, from, copy);
This is all going in the wrong direction entirely.
That "__copy_to_user()" code was bad from the beginning: it should never have had the double underscores. I objected to it at the time.
Now you're making it go from bad to insane. You're apparently mis-using "inatomic" because of subtle issues that have nothing to do with "inatomic" - you want to get rid of a might_sleep() warning, but you don't actuially want inatomic behavior, so the thing will still sleep.
This all very subtle already depends on people having checked the "struct iov_iter" beforehand. We should *remove* subtle stuff like that, not add yet more layers of subtlety and possible future bugs when somebody calls copy_page_to_iter() without having properly validated the iter.
These are not theoretical issues. We've _had_ these exact bugs when people didn't validate the stuff they created by hand and bypassed the normal IO paths.
Trying to optimize away an access_ok() or a might_fault() is *not* a valid reason to completely break our security model, and create code that makes no sense (claiming it is atomic when it isn't).
Linus
| |