| Message ID | a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin() | expand |
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
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...
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
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);
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(-)