Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567295 > unrolled thread

[PATCH 00/14] x86/fpu: Clean up ptrace copying functions

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/14] x86/fpu: Clean up ptrace copying functions Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
    [PATCH 06/14] x86/fpu: Clean up the parameter definitions of copy_xstate_to_*() Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
    [PATCH 05/14] x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIs Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
    [PATCH 13/14] x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
    [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
      Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the  copy_xstate_to_user() APIs Borislav Petkov <bp@alien8.de> - 2017-01-27 11:20 +0100
        Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the  copy_xstate_to_user() APIs Ingo Molnar <mingo@kernel.org> - 2017-01-30 11:10 +0100
          Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the  copy_xstate_to_user() APIs Borislav Petkov <bp@alien8.de> - 2017-01-30 16:50 +0100
            Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the  copy_xstate_to_user() APIs Yu-cheng Yu <yu-cheng.yu@intel.com> - 2017-01-30 18:30 +0100
    [PATCH 14/14] x86/fpu: Flip the parameter order in copy_*_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
    [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100
      Re: [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into  copy_kernel_to_xstate() & copy_user_to_xstate() Borislav Petkov <bp@alien8.de> - 2017-01-27 12:10 +0100
    [PATCH 02/14] x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user() Ingo Molnar <mingo@kernel.org> - 2017-01-26 11:30 +0100

#1567295 — [PATCH 00/14] x86/fpu: Clean up ptrace copying functions

This series deobfuscates the ptrace/regset handling functions of the x86 FPU
code. As a side effect it (should ...) fix the bug Rik reported in:

  [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy

but it does not handle the other fix yet:

  [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

... which needs to be rebased on top of this series.

The code got larger, but it is now a lot more standard and a lot easier to
understand as well.

Only very minimally tested: we should also add various ptrace XSAVES testcases
to tools/testing/selftests/x86/.

Obviously this will have to be backported as a single group of commits - but I'd rather
do that than leave the mess around.

Ingo Molnar (14):
  x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user()
  x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()
  x86/fpu: Remove 'ubuf' parameter from the copy_xstate_to_kernel() APIs
  x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
  x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIs
  x86/fpu: Clean up the parameter definitions of copy_xstate_to_*()
  x86/fpu: Remove the 'start_pos' parameter from the __copy_xstate_to_*() functions
  x86/fpu: Clarify parameter names in the copy_xstate_to_*() methods
  x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*()
  x86/fpu: Simplify __copy_xstate_to_kernel() return values
  x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate()
  x86/fpu: Remove 'ubuf' parameter from the copy_kernel_to_xstate() API
  x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API
  x86/fpu: Flip the parameter order in copy_*_to_xstate()

 arch/x86/include/asm/fpu/xstate.h |   8 +--
 arch/x86/kernel/fpu/regset.c      |  15 +++--
 arch/x86/kernel/fpu/signal.c      |  11 ++--
 arch/x86/kernel/fpu/xstate.c      | 188 ++++++++++++++++++++++++++++++++++++++++++++++------------
 4 files changed, 168 insertions(+), 54 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1567296 — [PATCH 06/14] x86/fpu: Clean up the parameter definitions of copy_xstate_to_*()

Remove pointless 'const' of non-pointer input parameter.

Remove unnecessary parenthesis that shows uncertainty about arithmetic operator precedence.

Clarify copy_xstate_to_user() description.

No change in functionality.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d2f4912c15e7..fd68a4e6427c 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -926,13 +926,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 static inline int
 __copy_xstate_to_kernel(void *kbuf,
 			const void *data,
-			unsigned int pos, unsigned int count, const int start_pos, const int end_pos)
+			unsigned int pos, unsigned int count, int start_pos, int end_pos)
 {
 	if ((count == 0) || (pos < start_pos))
 		return 0;
 
 	if (end_pos < 0 || pos < end_pos) {
-		unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));
+		unsigned int copy = end_pos < 0 ? count : min(count, end_pos - pos);
 
 		memcpy(kbuf + pos, data, copy);
 	}
@@ -1009,13 +1009,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int po
 }
 
 static inline int
-__copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int pos, unsigned int count, const int start_pos, const int end_pos)
+__copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int pos, unsigned int count, int start_pos, int end_pos)
 {
 	if ((count == 0) || (pos < start_pos))
 		return 0;
 
 	if (end_pos < 0 || pos < end_pos) {
-		unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));
+		unsigned int copy = end_pos < 0 ? count : min(count, end_pos - pos);
 
 		if (__copy_to_user(ubuf + pos, data, copy))
 			return -EFAULT;
@@ -1025,7 +1025,7 @@ __copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int pos, uns
 
 /*
  * Convert from kernel XSAVES compacted format to standard format and copy
- * to a ptrace buffer. It supports partial copy but pos always starts from
+ * to a user-space buffer. It supports partial copy but pos always starts from
  * zero. This is called from xstateregs_get() and there we check the CPU
  * has XSAVES.
  */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567297 — [PATCH 05/14] x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIs

Parameter ordering is weird:

  int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, struct xregs_state *xsave);
  int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, struct xregs_state *xsave);

'pos' and 'count', which are attributes of the destination buffer, are listed before the destination
buffer itself ...

List them after the primary arguments instead.

This makes the code more similar to regular memcpy() variant APIs.

No change in functionality.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |  4 ++--
 arch/x86/kernel/fpu/regset.c      |  4 ++--
 arch/x86/kernel/fpu/xstate.c      | 25 ++++++++++++-------------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 65bd68c30cd0..e4430b84939d 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
-int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, struct xregs_state *xsave);
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, struct xregs_state *xsave);
+int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int pos, unsigned int count);
+int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int pos, unsigned int count);
 int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 62a2d1154e45..72cf367627e2 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -92,9 +92,9 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 
 	if (using_compacted_format()) {
 		if (kbuf)
-			ret = copy_xstate_to_kernel(pos, count, kbuf, xsave);
+			ret = copy_xstate_to_kernel(kbuf, xsave, pos, count);
 		else
-			ret = copy_xstate_to_user(pos, count, ubuf, xsave);
+			ret = copy_xstate_to_user(ubuf, xsave, pos, count);
 	} else {
 		fpstate_sanitize_xstate(fpu);
 		/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1ac3d2ae73e6..d2f4912c15e7 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -924,10 +924,9 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
  * the source data pointer or increment pos, count, kbuf, and ubuf.
  */
 static inline int
-__copy_xstate_to_kernel(unsigned int pos, unsigned int count,
-			void *kbuf,
-			const void *data, const int start_pos,
-			const int end_pos)
+__copy_xstate_to_kernel(void *kbuf,
+			const void *data,
+			unsigned int pos, unsigned int count, const int start_pos, const int end_pos)
 {
 	if ((count == 0) || (pos < start_pos))
 		return 0;
@@ -947,7 +946,7 @@ __copy_xstate_to_kernel(unsigned int pos, unsigned int count,
  * It supports partial copy but pos always starts from zero. This is called
  * from xstateregs_get() and there we check the CPU has XSAVES.
  */
-int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, struct xregs_state *xsave)
+int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int pos, unsigned int count)
 {
 	unsigned int offset, size;
 	int ret, i;
@@ -972,7 +971,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
 	offset = offsetof(struct xregs_state, header);
 	size = sizeof(header);
 
-	ret = __copy_xstate_to_kernel(offset, size, kbuf, &header, 0, count);
+	ret = __copy_xstate_to_kernel(kbuf, &header, offset, size, 0, count);
 	if (ret)
 		return ret;
 
@@ -986,7 +985,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
 
-			ret = __copy_xstate_to_kernel(offset, size, kbuf, src, 0, count);
+			ret = __copy_xstate_to_kernel(kbuf, src, offset, size, 0, count);
 			if (ret)
 				return ret;
 
@@ -1002,7 +1001,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
 	offset = offsetof(struct fxregs_state, sw_reserved);
 	size = sizeof(xstate_fx_sw_bytes);
 
-	ret = __copy_xstate_to_kernel(offset, size, kbuf, xstate_fx_sw_bytes, 0, count);
+	ret = __copy_xstate_to_kernel(kbuf, xstate_fx_sw_bytes, offset, size, 0, count);
 	if (ret)
 		return ret;
 
@@ -1010,7 +1009,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
 }
 
 static inline int
-__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, const void *data, const int start_pos, const int end_pos)
+__copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int pos, unsigned int count, const int start_pos, const int end_pos)
 {
 	if ((count == 0) || (pos < start_pos))
 		return 0;
@@ -1030,7 +1029,7 @@ __copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, c
  * zero. This is called from xstateregs_get() and there we check the CPU
  * has XSAVES.
  */
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, struct xregs_state *xsave)
+int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int pos, unsigned int count)
 {
 	unsigned int offset, size;
 	int ret, i;
@@ -1055,7 +1054,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf,
 	offset = offsetof(struct xregs_state, header);
 	size = sizeof(header);
 
-	ret = __copy_xstate_to_user(offset, size, ubuf, &header, 0, count);
+	ret = __copy_xstate_to_user(ubuf, &header, offset, size, 0, count);
 	if (ret)
 		return ret;
 
@@ -1069,7 +1068,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf,
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
 
-			ret = __copy_xstate_to_user(offset, size, ubuf, src, 0, count);
+			ret = __copy_xstate_to_user(ubuf, src, offset, size, 0, count);
 			if (ret)
 				return ret;
 
@@ -1085,7 +1084,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf,
 	offset = offsetof(struct fxregs_state, sw_reserved);
 	size = sizeof(xstate_fx_sw_bytes);
 
-	ret = __copy_xstate_to_user(offset, size, ubuf, xstate_fx_sw_bytes, 0, count);
+	ret = __copy_xstate_to_user(ubuf, xstate_fx_sw_bytes, offset, size, 0, count);
 	if (ret)
 		return ret;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567298 — [PATCH 13/14] x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API

No change in functionality.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |  2 +-
 arch/x86/kernel/fpu/regset.c      |  2 +-
 arch/x86/kernel/fpu/signal.c      | 11 ++++-------
 arch/x86/kernel/fpu/xstate.c      | 19 +++++--------------
 4 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index f10889bc0c88..4ceb90740d80 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -51,5 +51,5 @@ int using_compacted_format(void);
 int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
 int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
 int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave);
-int copy_user_to_xstate(const void *kbuf, const void __user *ubuf, struct xregs_state *xsave);
+int copy_user_to_xstate(const void __user *ubuf, struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index ffcbed521fd9..f0c4a5ec7044 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -137,7 +137,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 		if (kbuf)
 			ret = copy_kernel_to_xstate(kbuf, xsave);
 		else
-			ret = copy_user_to_xstate(kbuf, ubuf, xsave);
+			ret = copy_user_to_xstate(ubuf, xsave);
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 	}
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index b1fe9a1fc4e0..2c685b492fd6 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -323,13 +323,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		fpu__drop(fpu);
 
-		if (using_compacted_format()) {
-			err = copy_user_to_xstate(NULL, buf_fx,
-					       &fpu->state.xsave);
-		} else {
-			err = __copy_from_user(&fpu->state.xsave,
-					       buf_fx, state_size);
-		}
+		if (using_compacted_format())
+			err = copy_user_to_xstate(buf_fx, &fpu->state.xsave);
+		else
+			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {
 			fpstate_init(&fpu->state);
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 3b89b2784665..1402a5496654 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1141,8 +1141,7 @@ int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave)
  * there we check the CPU has XSAVES and a whole standard-sized buffer
  * exists.
  */
-int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
-		     struct xregs_state *xsave)
+int copy_user_to_xstate(const void __user *ubuf, struct xregs_state *xsave)
 {
 	unsigned int offset, size;
 	int i;
@@ -1152,12 +1151,8 @@ int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 	offset = offsetof(struct xregs_state, header);
 	size = sizeof(xfeatures);
 
-	if (kbuf) {
-		memcpy(&xfeatures, kbuf + offset, size);
-	} else {
-		if (__copy_from_user(&xfeatures, ubuf + offset, size))
-			return -EFAULT;
-	}
+	if (__copy_from_user(&xfeatures, ubuf + offset, size))
+		return -EFAULT;
 
 	/*
 	 * Reject if the user sets any disabled or supervisor features:
@@ -1176,12 +1171,8 @@ int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
 
-			if (kbuf) {
-				memcpy(dst, kbuf + offset, size);
-			} else {
-				if (__copy_from_user(dst, ubuf + offset, size))
-					return -EFAULT;
-			}
+			if (__copy_from_user(dst, ubuf + offset, size))
+				return -EFAULT;
 		}
 	}
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567299 — [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

The 'kbuf' parameter is unused in the _user() side of the API, remove it.

This simplifies the code and makes it easier to think about.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |  2 +-
 arch/x86/kernel/fpu/regset.c      |  2 +-
 arch/x86/kernel/fpu/xstate.c      | 25 +++++++------------------
 3 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index c762574a245f..65bd68c30cd0 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -49,7 +49,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
 int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, struct xregs_state *xsave);
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf, struct xregs_state *xsave);
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, struct xregs_state *xsave);
 int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index a5f2b38a47dc..62a2d1154e45 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -94,7 +94,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 		if (kbuf)
 			ret = copy_xstate_to_kernel(pos, count, kbuf, xsave);
 		else
-			ret = copy_xstate_to_user(pos, count, kbuf, ubuf, xsave);
+			ret = copy_xstate_to_user(pos, count, ubuf, xsave);
 	} else {
 		fpstate_sanitize_xstate(fpu);
 		/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 6ac8e4a759ff..1ac3d2ae73e6 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
 }
 
 static inline int
-__copy_xstate_to_user(unsigned int pos, unsigned int count,
-		      void *kbuf, void __user *ubuf,
-		      const void *data, const int start_pos,
-		      const int end_pos)
+__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, const void *data, const int start_pos, const int end_pos)
 {
 	if ((count == 0) || (pos < start_pos))
 		return 0;
@@ -1021,12 +1018,8 @@ __copy_xstate_to_user(unsigned int pos, unsigned int count,
 	if (end_pos < 0 || pos < end_pos) {
 		unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));
 
-		if (kbuf) {
-			memcpy(kbuf + pos, data, copy);
-		} else {
-			if (__copy_to_user(ubuf + pos, data, copy))
-				return -EFAULT;
-		}
+		if (__copy_to_user(ubuf + pos, data, copy))
+			return -EFAULT;
 	}
 	return 0;
 }
@@ -1037,8 +1030,7 @@ __copy_xstate_to_user(unsigned int pos, unsigned int count,
  * zero. This is called from xstateregs_get() and there we check the CPU
  * has XSAVES.
  */
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
-			void __user *ubuf, struct xregs_state *xsave)
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, struct xregs_state *xsave)
 {
 	unsigned int offset, size;
 	int ret, i;
@@ -1063,8 +1055,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 	offset = offsetof(struct xregs_state, header);
 	size = sizeof(header);
 
-	ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, &header, 0, count);
-
+	ret = __copy_xstate_to_user(offset, size, ubuf, &header, 0, count);
 	if (ret)
 		return ret;
 
@@ -1078,8 +1069,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
 
-			ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, src, 0, count);
-
+			ret = __copy_xstate_to_user(offset, size, ubuf, src, 0, count);
 			if (ret)
 				return ret;
 
@@ -1095,8 +1085,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 	offset = offsetof(struct fxregs_state, sw_reserved);
 	size = sizeof(xstate_fx_sw_bytes);
 
-	ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, xstate_fx_sw_bytes, 0, count);
-
+	ret = __copy_xstate_to_user(offset, size, ubuf, xstate_fx_sw_bytes, 0, count);
 	if (ret)
 		return ret;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1568020 — Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> 
> This simplifies the code and makes it easier to think about.

...

> @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
>  }
>  
>  static inline int
> -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> -		      void *kbuf, void __user *ubuf,
> -		      const void *data, const int start_pos,
> -		      const int end_pos)
> +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, const void *data, const int start_pos, const int end_pos)

That and similar lines are insanely long and could be broken.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1569530 — Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> > The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> > 
> > This simplifies the code and makes it easier to think about.
> 
> ...
> 
> > @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
> >  }
> >  
> >  static inline int
> > -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> > -		      void *kbuf, void __user *ubuf,
> > -		      const void *data, const int start_pos,
> > -		      const int end_pos)
> > +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, const void *data, const int start_pos, const int end_pos)
> 
> That and similar lines are insanely long and could be broken.

Yeah, so that's one point of the series: the interface was insanely complex (the 
original sin is that of the regset interfaces), and one symptom of that complexity 
are these overly long prototypes - the above one has 7 arguments (!!). Another, 
far more serious symptom of the complexity were the bugs that Rik found.

The solution was not to break the prototype into multiple lines and thus paper 
over one symptom of complexity, but to _reduce_ complexity.

So at the end of the series the basic copy_xstate_to_user() prototype looks like 
this:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int offset, unsigned int size, unsigned int size_total)

which is less complex and shorter as well. It could probably be shortened further:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, u32 offset, u32 size, u32 total)

because our regset (and user-copy) APIs are intentionally 32-bit - but this would 
depart from the existing signature style so I'm not sure we want to do it 
unilaterally.

Would anyone object to using u32 in these prototypes?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1569849 — Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

On Mon, Jan 30, 2017 at 10:57:28AM +0100, Ingo Molnar wrote:
> Would anyone object to using u32 in these prototypes?

Well, would there be any disadvantage to forcing them to u32?
Potentially by something else wanting to use those interfaces besides
the regset thing and that something else doesn't like u32s?

Otherwise, I don't see a problem.

I mean, if 4G are not enough for xstate dimensions then we have a whole
different problem.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1569926 — Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs

On Mon, Jan 30, 2017 at 04:45:21PM +0100, Borislav Petkov wrote:
> On Mon, Jan 30, 2017 at 10:57:28AM +0100, Ingo Molnar wrote:
> > Would anyone object to using u32 in these prototypes?
> 
> Well, would there be any disadvantage to forcing them to u32?
> Potentially by something else wanting to use those interfaces besides
> the regset thing and that something else doesn't like u32s?
> 
> Otherwise, I don't see a problem.
> 
> I mean, if 4G are not enough for xstate dimensions then we have a whole
> different problem.

This function pair was intended to be similar to user_regset_copyout(), 
user_regset_copyin() used for the standard-format XSAVE area copying.
I totally agree it is complex and should be simplified.  Why don't we
do both places? 

Yu-cheng
 

[toc] | [prev] | [next] | [standalone]

#1567302 — [PATCH 14/14] x86/fpu: Flip the parameter order in copy_*_to_xstate()

Make it more consistent with regular memcpy() semantics, where the destination
argument comes first.

No change in functionality.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h | 4 ++--
 arch/x86/kernel/fpu/regset.c      | 4 ++--
 arch/x86/kernel/fpu/signal.c      | 2 +-
 arch/x86/kernel/fpu/xstate.c      | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 4ceb90740d80..579ac2358e63 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -50,6 +50,6 @@ const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
 int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
 int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
-int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave);
-int copy_user_to_xstate(const void __user *ubuf, struct xregs_state *xsave);
+int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
+int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index f0c4a5ec7044..b7f3e45ce15f 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -135,9 +135,9 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
 		if (kbuf)
-			ret = copy_kernel_to_xstate(kbuf, xsave);
+			ret = copy_kernel_to_xstate(xsave, kbuf);
 		else
-			ret = copy_user_to_xstate(ubuf, xsave);
+			ret = copy_user_to_xstate(xsave, ubuf);
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 	}
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 2c685b492fd6..2d682dac35d4 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -324,7 +324,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		fpu__drop(fpu);
 
 		if (using_compacted_format())
-			err = copy_user_to_xstate(buf_fx, &fpu->state.xsave);
+			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		else
 			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
 
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1402a5496654..772a069f8fbf 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1088,7 +1088,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
  * there we check the CPU has XSAVES and a whole standard-sized buffer
  * exists.
  */
-int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave)
+int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 {
 	unsigned int offset, size;
 	int i;
@@ -1141,7 +1141,7 @@ int copy_kernel_to_xstate(const void *kbuf, struct xregs_state *xsave)
  * there we check the CPU has XSAVES and a whole standard-sized buffer
  * exists.
  */
-int copy_user_to_xstate(const void __user *ubuf, struct xregs_state *xsave)
+int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 {
 	unsigned int offset, size;
 	int i;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1567303 — [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate()

Similar to:

  x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()

No change in functionality.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |  4 ++--
 arch/x86/kernel/fpu/regset.c      | 10 ++++++---
 arch/x86/kernel/fpu/xstate.c      | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index fed6617a1079..79af79dbcab6 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -50,6 +50,6 @@ const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
 int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
 int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
-int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
-		     struct xregs_state *xsave);
+int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf, struct xregs_state *xsave);
+int copy_user_to_xstate(const void *kbuf, const void __user *ubuf, struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 72cf367627e2..ea9e53d2bd8a 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -133,10 +133,14 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	fpu__activate_fpstate_write(fpu);
 
-	if (boot_cpu_has(X86_FEATURE_XSAVES))
-		ret = copy_user_to_xstate(kbuf, ubuf, xsave);
-	else
+	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+		if (kbuf)
+			ret = copy_kernel_to_xstate(kbuf, ubuf, xsave);
+		else
+			ret = copy_user_to_xstate(kbuf, ubuf, xsave);
+	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
+	}
 
 	/*
 	 * In case of failure, mark all states as init:
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7d24fe305d4b..aed9b2cb4d3d 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1083,7 +1083,71 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
 }
 
 /*
- * Convert from a ptrace standard-format buffer to kernel XSAVES format
+ * Convert from a ptrace standard-format kernel buffer to kernel XSAVES format
+ * and copy to the target thread. This is called from xstateregs_set() and
+ * there we check the CPU has XSAVES and a whole standard-sized buffer
+ * exists.
+ */
+int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf,
+		     struct xregs_state *xsave)
+{
+	unsigned int offset, size;
+	int i;
+	u64 xfeatures;
+	u64 allowed_features;
+
+	offset = offsetof(struct xregs_state, header);
+	size = sizeof(xfeatures);
+
+	if (kbuf) {
+		memcpy(&xfeatures, kbuf + offset, size);
+	} else {
+		if (__copy_from_user(&xfeatures, ubuf + offset, size))
+			return -EFAULT;
+	}
+
+	/*
+	 * Reject if the user sets any disabled or supervisor features:
+	 */
+	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
+
+	if (xfeatures & ~allowed_features)
+		return -EINVAL;
+
+	for (i = 0; i < XFEATURE_MAX; i++) {
+		u64 mask = ((u64)1 << i);
+
+		if (xfeatures & mask) {
+			void *dst = __raw_xsave_addr(xsave, 1 << i);
+
+			offset = xstate_offsets[i];
+			size = xstate_sizes[i];
+
+			if (kbuf) {
+				memcpy(dst, kbuf + offset, size);
+			} else {
+				if (__copy_from_user(dst, ubuf + offset, size))
+					return -EFAULT;
+			}
+		}
+	}
+
+	/*
+	 * The state that came in from userspace was user-state only.
+	 * Mask all the user states out of 'xfeatures':
+	 */
+	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;
+
+	/*
+	 * Add back in the features that came in from userspace:
+	 */
+	xsave->header.xfeatures |= xfeatures;
+
+	return 0;
+}
+
+/*
+ * Convert from a ptrace standard-format user-space buffer to kernel XSAVES format
  * and copy to the target thread. This is called from xstateregs_set() and
  * there we check the CPU has XSAVES and a whole standard-sized buffer
  * exists.
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1568077 — Re: [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate()

On Thu, Jan 26, 2017 at 11:22:56AM +0100, Ingo Molnar wrote:
> Similar to:
> 
>   x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()
> 
> No change in functionality.

...

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 7d24fe305d4b..aed9b2cb4d3d 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1083,7 +1083,71 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
>  }
>  
>  /*
> - * Convert from a ptrace standard-format buffer to kernel XSAVES format
> + * Convert from a ptrace standard-format kernel buffer to kernel XSAVES format
> + * and copy to the target thread. This is called from xstateregs_set() and
> + * there we check the CPU has XSAVES and a whole standard-sized buffer
> + * exists.
> + */
> +int copy_kernel_to_xstate(const void *kbuf, const void __user *ubuf,
> +		     struct xregs_state *xsave)

I'm wondering if we could avoid that code duplication in a cleaner way.
So that function is doing two things conceptually:

* get xfeatures
* iterate over them

It is being called by xstateregs_set(), for example. So you could do:

xstateregs_set:

	if (kbuf)
		copy_kernel_to_xstate()
	else
		copy_user_to_xstate()

Now those two could assign the copying function pointers:

	copy_fn = memcpy;

and
	copy_fn = __copy_from_user;

or a wrapper or whatever.

And then call a "workhorse" function:

	/* get_xfeatures */
	__copy_x_to_xstate(xstate, xfeatures, buf, copy_fn);

The buf thing would have to be cast to (void *) to drop the __user
annotation, though. Would that still be ok for sparse, I dunno?

Hmmm?

> +{
> +	unsigned int offset, size;
> +	int i;
> +	u64 xfeatures;
> +	u64 allowed_features;
> +
> +	offset = offsetof(struct xregs_state, header);
> +	size = sizeof(xfeatures);
> +
> +	if (kbuf) {
> +		memcpy(&xfeatures, kbuf + offset, size);
> +	} else {
> +		if (__copy_from_user(&xfeatures, ubuf + offset, size))
> +			return -EFAULT;
> +	}
> +
> +	/*
> +	 * Reject if the user sets any disabled or supervisor features:
> +	 */
> +	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
> +
> +	if (xfeatures & ~allowed_features)
> +		return -EINVAL;
> +
> +	for (i = 0; i < XFEATURE_MAX; i++) {
> +		u64 mask = ((u64)1 << i);
> +
> +		if (xfeatures & mask) {

So this mask is redundant - this could be done this way:

		if (xfeatures & BIT_ULL(i))

> +			void *dst = __raw_xsave_addr(xsave, 1 << i);

This thing looks yucky too:

we have a bit number, we convert it to a bit mask, in __raw_xsave_addr()
we convert it *back* to a bit number... are we testing the bitops speed
of the CPU or what's going on?

AFAICT, this interface could simply be converted to accepting bit
numbers and the callers of get_xsave_addr() could be simplified too.
From looking at fill_xsave() and load_xsave() in kvm, they already
generate the bit indices for CPUID - no need to generate the masks too.

Btw, __raw_xsave_addr() can be static too.

Btw 2,

void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)

	...

        WARN_ONCE(!(xfeatures_mask & xstate_feature),
                  "get of unsupported state");
		  ^^^^^^^^^^^^^^^^^^^^^^^^^

that's funny english.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1567304 — [PATCH 02/14] x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user()

copy_xstate_to_user() is a weird API - in part due to a bad API inherited
from the regset APIs.

But don't propagate that bad API choice into the FPU code - so as a first
step split the API into kernel and user buffer handling routines.

(Also split the xstate_copyout() internal helper.)

The split API is a dumb duplication that should be obviously correct, the
real splitting will be done in the next patch.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |   4 +--
 arch/x86/kernel/fpu/regset.c      |   5 ++-
 arch/x86/kernel/fpu/xstate.c      | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 109 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index a1baa17e9748..92dc8ca14124 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -48,8 +48,8 @@ void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
-int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
-			void __user *ubuf, struct xregs_state *xsave);
+int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf, struct xregs_state *xsave);
+int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf, struct xregs_state *xsave);
 int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 4efb81d6ee74..60a52a5e48c4 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -91,7 +91,10 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 	fpu__activate_fpstate_read(fpu);
 
 	if (using_compacted_format()) {
-		ret = copy_xstate_to_user(pos, count, kbuf, ubuf, xsave);
+		if (kbuf)
+			ret = copy_xstate_to_kernel(pos, count, kbuf, ubuf, xsave);
+		else
+			ret = copy_xstate_to_user(pos, count, kbuf, ubuf, xsave);
 	} else {
 		fpstate_sanitize_xstate(fpu);
 		/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index b0e471bf471c..8bd181f6f1f4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -923,10 +923,106 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
  * This is similar to user_regset_copyout(), but will not add offset to
  * the source data pointer or increment pos, count, kbuf, and ubuf.
  */
-static inline int xstate_copyout(unsigned int pos, unsigned int count,
-				 void *kbuf, void __user *ubuf,
-				 const void *data, const int start_pos,
-				 const int end_pos)
+static inline int
+__copy_xstate_to_kernel(unsigned int pos, unsigned int count,
+			void *kbuf, void __user *ubuf,
+			const void *data, const int start_pos,
+			const int end_pos)
+{
+	if ((count == 0) || (pos < start_pos))
+		return 0;
+
+	if (end_pos < 0 || pos < end_pos) {
+		unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));
+
+		if (kbuf) {
+			memcpy(kbuf + pos, data, copy);
+		} else {
+			if (__copy_to_user(ubuf + pos, data, copy))
+				return -EFAULT;
+		}
+	}
+	return 0;
+}
+
+/*
+ * Convert from kernel XSAVES compacted format to standard format and copy
+ * to a kernel-space ptrace buffer.
+ *
+ * It supports partial copy but pos always starts from zero. This is called
+ * from xstateregs_get() and there we check the CPU has XSAVES.
+ */
+int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf,
+			void __user *ubuf, struct xregs_state *xsave)
+{
+	unsigned int offset, size;
+	int ret, i;
+	struct xstate_header header;
+
+	/*
+	 * Currently copy_regset_to_user() starts from pos 0:
+	 */
+	if (unlikely(pos != 0))
+		return -EFAULT;
+
+	/*
+	 * The destination is a ptrace buffer; we put in only user xstates:
+	 */
+	memset(&header, 0, sizeof(header));
+	header.xfeatures = xsave->header.xfeatures;
+	header.xfeatures &= ~XFEATURE_MASK_SUPERVISOR;
+
+	/*
+	 * Copy xregs_state->header:
+	 */
+	offset = offsetof(struct xregs_state, header);
+	size = sizeof(header);
+
+	ret = __copy_xstate_to_kernel(offset, size, kbuf, ubuf, &header, 0, count);
+
+	if (ret)
+		return ret;
+
+	for (i = 0; i < XFEATURE_MAX; i++) {
+		/*
+		 * Copy only in-use xstates:
+		 */
+		if ((header.xfeatures >> i) & 1) {
+			void *src = __raw_xsave_addr(xsave, 1 << i);
+
+			offset = xstate_offsets[i];
+			size = xstate_sizes[i];
+
+			ret = __copy_xstate_to_kernel(offset, size, kbuf, ubuf, src, 0, count);
+
+			if (ret)
+				return ret;
+
+			if (offset + size >= count)
+				break;
+		}
+
+	}
+
+	/*
+	 * Fill xsave->i387.sw_reserved value for ptrace frame:
+	 */
+	offset = offsetof(struct fxregs_state, sw_reserved);
+	size = sizeof(xstate_fx_sw_bytes);
+
+	ret = __copy_xstate_to_kernel(offset, size, kbuf, ubuf, xstate_fx_sw_bytes, 0, count);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static inline int
+__copy_xstate_to_user(unsigned int pos, unsigned int count,
+		      void *kbuf, void __user *ubuf,
+		      const void *data, const int start_pos,
+		      const int end_pos)
 {
 	if ((count == 0) || (pos < start_pos))
 		return 0;
@@ -976,7 +1072,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 	offset = offsetof(struct xregs_state, header);
 	size = sizeof(header);
 
-	ret = xstate_copyout(offset, size, kbuf, ubuf, &header, 0, count);
+	ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, &header, 0, count);
 
 	if (ret)
 		return ret;
@@ -991,7 +1087,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];
 
-			ret = xstate_copyout(offset, size, kbuf, ubuf, src, 0, count);
+			ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, src, 0, count);
 
 			if (ret)
 				return ret;
@@ -1008,7 +1104,7 @@ int copy_xstate_to_user(unsigned int pos, unsigned int count, void *kbuf,
 	offset = offsetof(struct fxregs_state, sw_reserved);
 	size = sizeof(xstate_fx_sw_bytes);
 
-	ret = xstate_copyout(offset, size, kbuf, ubuf, xstate_fx_sw_bytes, 0, count);
+	ret = __copy_xstate_to_user(offset, size, kbuf, ubuf, xstate_fx_sw_bytes, 0, count);
 
 	if (ret)
 		return ret;
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web