x86/fpu: Use fault_in_pages_writeable() for pre-faulting

Commit Message

Sebastian Andrzej Siewior May 26, 2019, 5:33 p.m. UTC

From: Hugh Dickins <[e-mail]>

Since commit

   d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")

we use get_user_pages_unlocked() to pre-faulting user's memory if a
write generates a page fault while the handler is disabled.
This works in general and uncovered a bug as reported by Mike Rapoport.

It has been pointed out that this function may be fragile and a
simple pre-fault as in fault_in_pages_writeable() would be a better
solution. Better as in taste and simplicity: That write (as performed by
the alternative function) performs exactly the same faulting of memory
that we had before. This was suggested by Hugh Dickins and Andrew
Morton.

Use fault_in_pages_writeable() for pre-faulting of user's stack.

Suggested-by: Andrew Morton <[e-mail]>
Signed-off-by: Hugh Dickins <[e-mail]>
Link: https://lkml.kernel.org/r/[e-mail]
[bigeasy: patch description]
Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
---
 arch/x86/kernel/fpu/signal.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Comments

Sebastian Andrzej Siewior May 26, 2019, 5:35 p.m. UTC | #1

On 2019-05-26 19:33:25 [+0200], To Hugh Dickins wrote:
> From: Hugh Dickins <[e-mail]>
…
> Signed-off-by: Hugh Dickins <[e-mail]>

Hugh, I took your patch, slapped a signed-off-by line. Please say that
you are fine with it (or object otherwise).

Sebastian

Hugh Dickins May 26, 2019, 7:25 p.m. UTC | #2

On Sun, 26 May 2019, Sebastian Andrzej Siewior wrote:
> On 2019-05-26 19:33:25 [+0200], To Hugh Dickins wrote:
> From: Hugh Dickins <[e-mail]>
> …
> > Signed-off-by: Hugh Dickins <[e-mail]>
> 
> Hugh, I took your patch, slapped a signed-off-by line. Please say that
> you are fine with it (or object otherwise).

I'm fine with it, thanks Sebastian. Sorry if I wasted your time by not
giving it my sign-off in the first place, but I was not comfortable to
dabble there without your sign-off too - which it now has. (And thought
you might already have your own version anyway: just provided mine as
illustration, so that we could be sure of exactly what I'd been testing.)

Hugh

Andrew Morton May 29, 2019, 4:18 a.m. UTC | #3

On Sun, 26 May 2019 19:33:25 +0200 Sebastian Andrzej Siewior <[e-mail]> wrote:

> From: Hugh Dickins <[e-mail]>
> 
> Since commit
> 
>    d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")

Please add this as a

Fixes: d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")

line so that anyone who backports d9c9ce34ed5c8 has a chance of finding
this patch also.

Patch

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 5a8d118bc423e..060d6188b4533 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -5,6 +5,7 @@ 
 
 #include <linux/compat.h>
 #include <linux/cpu.h>
+#include <linux/pagemap.h>
 
 #include <asm/fpu/internal.h>
 #include <asm/fpu/signal.h>
@@ -189,15 +190,7 @@  int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 	fpregs_unlock();
 
 	if (ret) {
-		int aligned_size;
-		int nr_pages;
-
-		aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
-		nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
-
-		ret = get_user_pages_unlocked((unsigned long)buf_fx, nr_pages,
-					      NULL, FOLL_WRITE);
-		if (ret == nr_pages)
+		if (!fault_in_pages_writeable(buf_fx, fpu_user_xstate_size))
 			goto retry;
 		return -EFAULT;
 	}