| Message ID | 20200509234557.1124086-5-viro@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/20] dlmfs_file_write(): get rid of pointless access_ok() | expand |
Hello, Al. I think that this access_ok() check helps reducing partial writes (either "whole amount was processed" or "not processed at all" unless -ENOMEM). Do you think that such attempt is pointless? Then, please go ahead... On 2020/05/10 8:45, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > address is passed only to get_user() > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > security/tomoyo/common.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c > index 1b467381986f..f93f8acd05f7 100644 > --- a/security/tomoyo/common.c > +++ b/security/tomoyo/common.c > @@ -2662,8 +2662,6 @@ ssize_t tomoyo_write_control(struct tomoyo_io_buffer *head, > > if (!head->write) > return -EINVAL; > - if (!access_ok(buffer, buffer_len)) > - return -EFAULT; > if (mutex_lock_interruptible(&head->io_sem)) > return -EINTR; > head->read_user_buf_avail = 0; >
On Sat, May 9, 2020 at 5:51 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > I think that this access_ok() check helps reducing partial writes (either > "whole amount was processed" or "not processed at all" unless -ENOMEM). No it doesn't. "access_ok()" only checks the range being a valid user address range. It doesn't actually help at all if the worry is "what if we take a page fault in the middle". Because it simply doesn't check those kinds of things. Now, if somebody passes actual invalid ranges (ie kernel addresses or other crazy stuff), they only have themselves to blame. The invalid range will be noticed when actually doing the user copy, and then you'll get EFAULT there. But there's no point in trying to figure that out early - it's only adding overhead, and it doesn't help any normal case. Linus
On 2020/05/10 9:57, Linus Torvalds wrote: > On Sat, May 9, 2020 at 5:51 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> I think that this access_ok() check helps reducing partial writes (either >> "whole amount was processed" or "not processed at all" unless -ENOMEM). > > No it doesn't. > > "access_ok()" only checks the range being a valid user address range. > I see. Thank you. Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
On Sat, May 09, 2020 at 05:57:56PM -0700, Linus Torvalds wrote: > On Sat, May 9, 2020 at 5:51 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > > I think that this access_ok() check helps reducing partial writes (either > > "whole amount was processed" or "not processed at all" unless -ENOMEM). > > No it doesn't. > > "access_ok()" only checks the range being a valid user address range. > > It doesn't actually help at all if the worry is "what if we take a > page fault in the middle". Because it simply doesn't check those > kinds of things. > > Now, if somebody passes actual invalid ranges (ie kernel addresses or > other crazy stuff), they only have themselves to blame. The invalid > range will be noticed when actually doing the user copy, and then > you'll get EFAULT there. But there's no point in trying to figure that > out early - it's only adding overhead, and it doesn't help any normal > case. It might be a good idea to add Documentation/what-access_ok-does_not ;-/ In addition to what you've mentioned, * access_ok() does not fault anything in; never had. * access_ok() does not verify that memory is readable/writable/there at all; never had, except for genuine 80386 and (maybe) some of the shittier 486 clones. * access_ok() does not protect you from the length being insanely large; even on i386 it can pass with length being a bit under 3Gb. If you count upon it to prevent kmalloc() complaints about insanely large allocation (yes, I've seen that excuse used), you are wrong. * on a bunch of architectures access_ok() never rejects anything, and no, that's _not_ MMU-less ones. sparc64, for example. Or s390, or parisc, etc.
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c index 1b467381986f..f93f8acd05f7 100644 --- a/security/tomoyo/common.c +++ b/security/tomoyo/common.c @@ -2662,8 +2662,6 @@ ssize_t tomoyo_write_control(struct tomoyo_io_buffer *head, if (!head->write) return -EINVAL; - if (!access_ok(buffer, buffer_len)) - return -EFAULT; if (mutex_lock_interruptible(&head->io_sem)) return -EINTR; head->read_user_buf_avail = 0;