Re: [PATCH] Convert filldir[64]() from __put_user() to unsafe_put_user()

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

On Sun, Oct 6, 2019 at 4:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Ho humm. I've run variations of that patch over a few years on x86,
> but obviously not on alpha/sparc.

Oooh.

I wonder... This may be the name string copy loop. And it's special in
that the result may not be aligned.

Now, a "__put_user()" with an unaligned address _should_ work - it's
very easy to trigger that from user space by just giving an unaligned
address to any system call that then writes a single word.

But alpha does

  #define __put_user_32(x, addr)                                  \
  __asm__ __volatile__("1: stl %r2,%1\n"                          \
          "2:\n"                                                  \
          EXC(1b,2b,$31,%0)                                       \
                  : "=r"(__pu_err)                                \
                  : "m"(__m(addr)), "rJ"(x), "0"(__pu_err))

iow it implements that 32-bit __put_user() as a 'stl'.

Which will trap if it's not aligned.

And I wonder how much testing that has ever gotten. Nobody really does
unaigned accesses on alpha.

We need to do that memcpy unrolling on x86, because x86 actually uses
"user_access_begin()" and we have magic rules about what is inside
that region.

But on alpha (and sparc) it might be better to just do "__copy_to_user()".

Anyway, this does look like a possible latent bug where the alpha
unaligned trap doesn't then handle the case of exceptions. I know it
_tries_, but I doubt it's gotten a whole lot of testing.

Anyway, let me think about this, but just for testing, does the
attached patch make any difference? It's not the right thing in
general (and most definitely not on x86), but for testing whether this
is about unaligned accesses it might work.

It's entirely untested, and in fact on x86 it should cause objtool to
complain about a function call with AC set. But I think that on alpha
and sparc, using __copy_to_user() for the name copy should work, and
would work around the unaligned issue.

That said, if it *is* the unaligned issue, then that just means that
we have a serious bug elsewhere in the alpha port. Maybe nobody cares.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 658 bytes --]

 fs/readdir.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/readdir.c b/fs/readdir.c
index 19bea591c3f1..d49c4e2c66a8 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -76,6 +76,15 @@
 	unsafe_put_user(0, dst, label);				\
 } while (0)
 
+/* Alpha (and sparc?) test patch! */
+#undef unsafe_copy_dirent_name
+#define unsafe_copy_dirent_name(_dst, _src, _len, label) do {	\
+	char __user *dst = (_dst);				\
+	const char *src = (_src);				\
+	size_t len = (_len);					\
+	if (__copy_to_user(dst, src, len)) goto label;		\
+	unsafe_put_user(0, dst+len, label);			\
+} while (0)
 
 int iterate_dir(struct file *file, struct dir_context *ctx)
 {