linux-kernel - Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 17 Apr 2021 20:30:42 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH] x86/uaccess: small optimization in unsafe_copy_to_user()

[tytso Cc'd]

On Sat, Apr 17, 2021 at 06:09:44PM +0000, Al Viro wrote:

> > Al - fairly trivial patch applied, comments?
> 
> Should be fine...  FWIW, I've a patch in the same area, making those suckers
> return bool.  Seeing that they are only ever called via dir_emit(), dir_emit_dot()
> and dir_emit_dotdot() and all of those return ->actor(...) == 0...
> 
> Anyway, that'd be trivial to rebase on top of yours.

Actually... looking through that patch now I've noticed something fishy:
in 1f60fbe72749 ("ext4: allow readdir()'s of large empty directories to
be interrupted" we had
@@ -169,6 +169,8 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
        }
        dirent = buf->previous;
        if (dirent) {
+               if (signal_pending(current))
+                       return -EINTR;

and that thing is still there.  However, it does *NOT* do what it might
appear to be doing; it ends up with getdents() returning -EINVAL instead.
What we need is
			buf->error = -EINTR;
in addition to that return (in all 3 such places).  Do you have any problems with
the following delta?

diff --git a/fs/readdir.c b/fs/readdir.c
index 19434b3c982c..c742db935847 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -239,8 +239,10 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
 		return -EOVERFLOW;
 	}
 	prev_reclen = buf->prev_reclen;
-	if (prev_reclen && signal_pending(current))
+	if (prev_reclen && signal_pending(current)) {
+		buf->error = -EINTR;
 		return -EINTR;
+	}
 	dirent = buf->current_dir;
 	prev = (void __user *) dirent - prev_reclen;
 	if (!user_write_access_begin(prev, reclen + prev_reclen))
@@ -321,8 +323,10 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
 	if (reclen > buf->count)
 		return -EINVAL;
 	prev_reclen = buf->prev_reclen;
-	if (prev_reclen && signal_pending(current))
+	if (prev_reclen && signal_pending(current)) {
+		buf->error = -EINTR;
 		return -EINTR;
+	}
 	dirent = buf->current_dir;
 	prev = (void __user *)dirent - prev_reclen;
 	if (!user_write_access_begin(prev, reclen + prev_reclen))
@@ -488,8 +492,10 @@ static int compat_filldir(struct dir_context *ctx, const char *name, int namlen,
 		return -EOVERFLOW;
 	}
 	prev_reclen = buf->prev_reclen;
-	if (prev_reclen && signal_pending(current))
+	if (prev_reclen && signal_pending(current)) {
+		buf->error = -EINTR;
 		return -EINTR;
+	}
 	dirent = buf->current_dir;
 	prev = (void __user *) dirent - prev_reclen;
 	if (!user_write_access_begin(prev, reclen + prev_reclen))