[05/20] tomoyo_write_control(): get rid of pointless access_ok()
diff mbox series

Message ID 20200509234557.1124086-5-viro@ZenIV.linux.org.uk
State New
Headers show
Series
  • [01/20] dlmfs_file_write(): get rid of pointless access_ok()
Related show

Commit Message

Al Viro May 9, 2020, 11:45 p.m. UTC
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(-)

Comments

Tetsuo Handa May 10, 2020, 12:50 a.m. UTC | #1
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;
>
Linus Torvalds May 10, 2020, 12:57 a.m. UTC | #2
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
Tetsuo Handa May 10, 2020, 1:04 a.m. UTC | #3
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>
Al Viro May 10, 2020, 3:01 a.m. UTC | #4
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.

Patch
diff mbox series

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;