[v1,1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
diff mbox series

Message ID a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr
State New
Headers show
Series
  • [v1,1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
Related show

Commit Message

Christophe Leroy Jan. 22, 2020, 1 p.m. UTC
Some architectures grand full access to userspace regardless of the
address/len passed to user_access_begin(), but other architectures
only grand access to the requested area.

For exemple, on 32 bits powerpc (book3s/32), access is granted by
segments of 256 Mbytes.

Modify filldir() and filldir64() to request the real area they need
to get access to.

Fixes: 9f79b78ef744 ("Convert filldir[64]() from __put_user() to unsafe_put_user()")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 fs/readdir.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

Comments

Linus Torvalds Jan. 22, 2020, 4:13 p.m. UTC | #1
On Wed, Jan 22, 2020 at 5:00 AM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
> Modify filldir() and filldir64() to request the real area they need
> to get access to.

Not like this.

This makes the situation for architectures like x86 much worse, since
you now use "put_user()" for the previous dirent filling. Which does
that expensive user access setup/teardown twice again.

So either you need to cover both the dirent's with one call, or you
just need to cover the whole (original) user buffer passed in. But not
this unholy mixing of both unsafe_put_user() and regular put_user().

              Linus
Al Viro Jan. 22, 2020, 5:41 p.m. UTC | #2
On Wed, Jan 22, 2020 at 08:13:12AM -0800, Linus Torvalds wrote:
> On Wed, Jan 22, 2020 at 5:00 AM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
> >
> > Modify filldir() and filldir64() to request the real area they need
> > to get access to.
> 
> Not like this.
> 
> This makes the situation for architectures like x86 much worse, since
> you now use "put_user()" for the previous dirent filling. Which does
> that expensive user access setup/teardown twice again.
> 
> So either you need to cover both the dirent's with one call, or you
> just need to cover the whole (original) user buffer passed in. But not
> this unholy mixing of both unsafe_put_user() and regular put_user().

I would suggest simply covering the range from dirent->d_off to
buf->current_dir->d_name[namelen]; they are going to be close to
each other and we need those addresses anyway...
Christophe Leroy Jan. 22, 2020, 5:54 p.m. UTC | #3
Le 22/01/2020 à 18:41, Al Viro a écrit :
> On Wed, Jan 22, 2020 at 08:13:12AM -0800, Linus Torvalds wrote:
>> On Wed, Jan 22, 2020 at 5:00 AM Christophe Leroy
>> <christophe.leroy@c-s.fr> wrote:
>>>
>>> Modify filldir() and filldir64() to request the real area they need
>>> to get access to.
>>
>> Not like this.
>>
>> This makes the situation for architectures like x86 much worse, since
>> you now use "put_user()" for the previous dirent filling. Which does
>> that expensive user access setup/teardown twice again.
>>
>> So either you need to cover both the dirent's with one call, or you
>> just need to cover the whole (original) user buffer passed in. But not
>> this unholy mixing of both unsafe_put_user() and regular put_user().
> 
> I would suggest simply covering the range from dirent->d_off to
> buf->current_dir->d_name[namelen]; they are going to be close to
> each other and we need those addresses anyway...
> 

In v2, I'm covering from the beginning of parent dirent to the end of 
current dirent.

Christophe

Patch
diff mbox series

diff --git a/fs/readdir.c b/fs/readdir.c
index d26d5ea4de7b..ef04e5e76c59 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -236,15 +236,11 @@  static int filldir(struct dir_context *ctx, const char *name, int namlen,
 	if (dirent && signal_pending(current))
 		return -EINTR;
 
-	/*
-	 * Note! This range-checks 'previous' (which may be NULL).
-	 * The real range was checked in getdents
-	 */
-	if (!user_access_begin(dirent, sizeof(*dirent)))
+	if (dirent && unlikely(put_user(offset, &dirent->d_off)))
 		goto efault;
-	if (dirent)
-		unsafe_put_user(offset, &dirent->d_off, efault_end);
 	dirent = buf->current_dir;
+	if (!user_access_begin(dirent, reclen))
+		goto efault;
 	unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
 	unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
 	unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
@@ -323,15 +319,11 @@  static int filldir64(struct dir_context *ctx, const char *name, int namlen,
 	if (dirent && signal_pending(current))
 		return -EINTR;
 
-	/*
-	 * Note! This range-checks 'previous' (which may be NULL).
-	 * The real range was checked in getdents
-	 */
-	if (!user_access_begin(dirent, sizeof(*dirent)))
+	if (dirent && unlikely(put_user(offset, &dirent->d_off)))
 		goto efault;
-	if (dirent)
-		unsafe_put_user(offset, &dirent->d_off, efault_end);
 	dirent = buf->current_dir;
+	if (!user_access_begin(dirent, reclen))
+		goto efault;
 	unsafe_put_user(ino, &dirent->d_ino, efault_end);
 	unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
 	unsafe_put_user(d_type, &dirent->d_type, efault_end);