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


Groups > linux.kernel > #1738139 > unrolled thread

[PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header"

Started byIngo Molnar <mingo@kernel.org>
First post2017-09-24 13:00 +0200
Last post2017-09-24 21:10 +0200
Articles 20 on this page of 28 — 4 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header" Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:00 +0200
    [PATCH 02/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:00 +0200
      [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the  xstate_header in xstateregs_set() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 09/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:00 +0200
      [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the  xstate_header in copy_user_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 10/10] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      [tip:x86/fpu] x86/fpu: Use using_compacted_format() instead of open  coded X86_FEATURE_XSAVES tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Eric Biggers <ebiggers3@gmail.com> - 2017-09-24 21:00 +0200
        Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 21:10 +0200
          Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Eric Biggers <ebiggers3@gmail.com> - 2017-09-24 22:10 +0200
            Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-25 08:10 +0200
            Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-25 08:20 +0200
              Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Eric Biggers <ebiggers3@gmail.com> - 2017-09-25 09:30 +0200
                Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate  the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-25 09:40 +0200
      [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the  xstate_header in __fpu__restore_sig() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 07/10] x86/fpu: Copy the full header in copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      [tip:x86/fpu] x86/fpu: Copy the full header in  copy_user_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 01/10] x86/fpu: Introduce validate_xstate_header() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      [tip:x86/fpu] x86/fpu: Introduce validate_xstate_header() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 08/10] x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      [tip:x86/fpu] x86/fpu: Eliminate the 'xfeatures' local variable in  copy_user_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 04/10] x86/fpu: Copy the full state_header in copy_kernel_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      [tip:x86/fpu] x86/fpu: Copy the full state_header in  copy_kernel_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    [PATCH 06/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
      [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the  xstate_header in copy_kernel_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
    Re: [PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of  user-supplied xstate_header" Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-24 20:10 +0200
      Re: [PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of  user-supplied xstate_header" Ingo Molnar <mingo@kernel.org> - 2017-09-24 21:10 +0200

Page 1 of 2  [1] 2  Next page →


#1738139 — [PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header"

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:00 +0200
Subject[PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header"
Message-ID<utcNH-7Gh-3@gated-at.bofh.it>
As mentioned before, the patch was too big and too complex, and I've split it
up into 10 smaller, bisectable patches:

Eric Biggers (10):
  x86/fpu: Introduce validate_xstate_header()
  x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()
  x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
  x86/fpu: Copy the full state_header in copy_kernel_to_xstate()
  x86/fpu: Eliminate the 'xfeatures' local variable in copy_kernel_to_xstate()
  x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate()
  x86/fpu: Copy the full header in copy_user_to_xstate()
  x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate()
  x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()
  x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES

 arch/x86/include/asm/fpu/xstate.h |  4 ++++
 arch/x86/kernel/fpu/regset.c      | 21 ++++++-----------
 arch/x86/kernel/fpu/signal.c      | 17 ++++++-------
 arch/x86/kernel/fpu/xstate.c      | 76 +++++++++++++++++++++++++++++++++--------------------------
 4 files changed, 63 insertions(+), 55 deletions(-)

I kept the attribution in place, because the end result is almost the same.

Below is the interdiff from the original version, I uninlined validate_xstate_header(),
because it's way too large to be inlined everywhere, plus I removed a couple of
spurious "!= 0" patterns.

The latest x86/fpu bits can be found in -tip:

    git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
    git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.x86/fpu

Thanks,

    Ingo

===
 include/asm/fpu/xstate.h |   23 +----------------------
 kernel/fpu/xstate.c      |   28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 3d79d0ee4d30..83fee2469eb7 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -54,27 +54,6 @@ 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);
 
 /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-static inline int validate_xstate_header(const struct xstate_header *hdr)
-{
-	/* No unknown or supervisor features may be set */
-	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
-		return -EINVAL;
-
-	/* Userspace must use the uncompacted format */
-	if (hdr->xcomp_bv)
-		return -EINVAL;
-
-	/*
-	 * If 'reserved' is shrunken to add a new field, make sure to validate
-	 * that new field here!
-	 */
-	BUILD_BUG_ON(sizeof(hdr->reserved) != 48);
-
-	/* No reserved bits may be set */
-	if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
-		return -EINVAL;
-
-	return 0;
-}
+extern int validate_xstate_header(const struct xstate_header *hdr);
 
 #endif
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7ebd1a0811b6..f1d5476c9022 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -483,6 +483,30 @@ int using_compacted_format(void)
 	return boot_cpu_has(X86_FEATURE_XSAVES);
 }
 
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+int validate_xstate_header(const struct xstate_header *hdr)
+{
+	/* No unknown or supervisor features may be set */
+	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
+		return -EINVAL;
+
+	/* Userspace must use the uncompacted format */
+	if (hdr->xcomp_bv)
+		return -EINVAL;
+
+	/*
+	 * If 'reserved' is shrunken to add a new field, make sure to validate
+	 * that new field here!
+	 */
+	BUILD_BUG_ON(sizeof(hdr->reserved) != 48);
+
+	/* No reserved bits may be set */
+	if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
+		return -EINVAL;
+
+	return 0;
+}
+
 static void __xstate_dump_leaves(void)
 {
 	int i;
@@ -1127,7 +1151,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 
 	memcpy(&hdr, kbuf + offset, size);
 
-	if (validate_xstate_header(&hdr) != 0)
+	if (validate_xstate_header(&hdr))
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {
@@ -1181,7 +1205,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	if (__copy_from_user(&hdr, ubuf + offset, size))
 		return -EFAULT;
 
-	if (validate_xstate_header(&hdr) != 0)
+	if (validate_xstate_header(&hdr))
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {

[toc] | [next] | [standalone]


#1738140 — [PATCH 02/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:00 +0200
Subject[PATCH 02/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()
Message-ID<utcNH-7Gh-5@gated-at.bofh.it>
In reply to#1738139
From: Eric Biggers <ebiggers@google.com>

Tighten the checks in xstateregs_set().

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/regset.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index ee8d2f049818..b831d5b9de99 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -141,27 +141,20 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 			ret = copy_user_to_xstate(xsave, ubuf);
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
-
-		/* xcomp_bv must be 0 when using uncompacted format */
-		if (!ret && xsave->header.xcomp_bv)
-			ret = -EINVAL;
+		if (!ret)
+			ret = validate_xstate_header(&xsave->header);
 	}
 
 	/*
-	 * In case of failure, mark all states as init:
-	 */
-	if (ret)
-		fpstate_init(&fpu->state);
-
-	/*
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
 	xsave->i387.mxcsr &= mxcsr_feature_mask;
-	xsave->header.xfeatures &= xfeatures_mask;
+
 	/*
-	 * These bits must be zero.
+	 * In case of failure, mark all states as init:
 	 */
-	memset(&xsave->header.reserved, 0, 48);
+	if (ret)
+		fpstate_init(&fpu->state);
 
 	return ret;
 }
-- 
2.11.0

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


#1739710 — [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()

Fromtip-bot for Eric Biggers <tipbot@zytor.com>
Date2017-09-26 10:50 +0200
Subject[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()
Message-ID<utTJ0-2nL-31@gated-at.bofh.it>
In reply to#1738140
Commit-ID:  cf9df81b139b6ebaec188d73758f02ca3b2110e4
Gitweb:     http://git.kernel.org/tip/cf9df81b139b6ebaec188d73758f02ca3b2110e4
Author:     Eric Biggers <ebiggers@google.com>
AuthorDate: Sun, 24 Sep 2017 12:59:05 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 26 Sep 2017 09:43:45 +0200

x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()

Tighten the checks in xstateregs_set().

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Link: http://lkml.kernel.org/r/20170924105913.9157-3-mingo@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/regset.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index ee8d2f0..b831d5b 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -141,27 +141,20 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 			ret = copy_user_to_xstate(xsave, ubuf);
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
-
-		/* xcomp_bv must be 0 when using uncompacted format */
-		if (!ret && xsave->header.xcomp_bv)
-			ret = -EINVAL;
+		if (!ret)
+			ret = validate_xstate_header(&xsave->header);
 	}
 
 	/*
-	 * In case of failure, mark all states as init:
-	 */
-	if (ret)
-		fpstate_init(&fpu->state);
-
-	/*
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
 	xsave->i387.mxcsr &= mxcsr_feature_mask;
-	xsave->header.xfeatures &= xfeatures_mask;
+
 	/*
-	 * These bits must be zero.
+	 * In case of failure, mark all states as init:
 	 */
-	memset(&xsave->header.reserved, 0, 48);
+	if (ret)
+		fpstate_init(&fpu->state);
 
 	return ret;
 }

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


#1738141 — [PATCH 09/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:00 +0200
Subject[PATCH 09/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()
Message-ID<utcNH-7Gh-9@gated-at.bofh.it>
In reply to#1738139
From: Eric Biggers <ebiggers@google.com>

Tighten the checks in copy_user_to_xstate().

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index b6d78b78b5c2..f1d5476c9022 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1188,16 +1188,15 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 }
 
 /*
- * 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.
+ * Convert from a ptrace or sigreturn standard-format user-space buffer to
+ * kernel XSAVES format and copy to the target thread. This is called from
+ * xstateregs_set(), as well as potentially from the sigreturn() and
+ * rt_sigreturn() system calls.
  */
 int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 {
 	unsigned int offset, size;
 	int i;
-	u64 allowed_features;
 	struct xstate_header hdr;
 
 	offset = offsetof(struct xregs_state, header);
@@ -1206,12 +1205,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	if (__copy_from_user(&hdr, ubuf + offset, size))
 		return -EFAULT;
 
-	/*
-	 * Reject if the user sets any disabled or supervisor features:
-	 */
-	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
-
-	if (hdr.xfeatures & ~allowed_features)
+	if (validate_xstate_header(&hdr))
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {
-- 
2.11.0

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


#1739711 — [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()

Fromtip-bot for Eric Biggers <tipbot@zytor.com>
Date2017-09-26 10:50 +0200
Subject[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()
Message-ID<utTJ0-2nL-33@gated-at.bofh.it>
In reply to#1738141
Commit-ID:  98c0fad9d60e8b2cd47e15b7bee7df343648f5bb
Gitweb:     http://git.kernel.org/tip/98c0fad9d60e8b2cd47e15b7bee7df343648f5bb
Author:     Eric Biggers <ebiggers@google.com>
AuthorDate: Sun, 24 Sep 2017 12:59:12 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 26 Sep 2017 09:43:48 +0200

x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()

Tighten the checks in copy_user_to_xstate().

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Link: http://lkml.kernel.org/r/20170924105913.9157-10-mingo@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index b6d78b7..f1d5476 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1188,16 +1188,15 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 }
 
 /*
- * 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.
+ * Convert from a ptrace or sigreturn standard-format user-space buffer to
+ * kernel XSAVES format and copy to the target thread. This is called from
+ * xstateregs_set(), as well as potentially from the sigreturn() and
+ * rt_sigreturn() system calls.
  */
 int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 {
 	unsigned int offset, size;
 	int i;
-	u64 allowed_features;
 	struct xstate_header hdr;
 
 	offset = offsetof(struct xregs_state, header);
@@ -1206,12 +1205,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	if (__copy_from_user(&hdr, ubuf + offset, size))
 		return -EFAULT;
 
-	/*
-	 * Reject if the user sets any disabled or supervisor features:
-	 */
-	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
-
-	if (hdr.xfeatures & ~allowed_features)
+	if (validate_xstate_header(&hdr))
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {

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


#1738142 — [PATCH 10/10] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:10 +0200
Subject[PATCH 10/10] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES
Message-ID<utcXn-7Yz-1@gated-at.bofh.it>
In reply to#1738139
From: Eric Biggers <ebiggers@google.com>

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/regset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index b831d5b9de99..3ea151372389 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -134,7 +134,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	fpu__prepare_write(fpu);
 
-	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+	if (using_compacted_format()) {
 		if (kbuf)
 			ret = copy_kernel_to_xstate(xsave, kbuf);
 		else
-- 
2.11.0

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


#1739698 — [tip:x86/fpu] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES

Fromtip-bot for Eric Biggers <tipbot@zytor.com>
Date2017-09-26 10:50 +0200
Subject[tip:x86/fpu] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES
Message-ID<utTIZ-2nL-3@gated-at.bofh.it>
In reply to#1738142
Commit-ID:  738f48cb5fdd5878d11934f1898aa2bcf1578289
Gitweb:     http://git.kernel.org/tip/738f48cb5fdd5878d11934f1898aa2bcf1578289
Author:     Eric Biggers <ebiggers@google.com>
AuthorDate: Sun, 24 Sep 2017 12:59:13 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 26 Sep 2017 09:43:48 +0200

x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES

This is the canonical method to use.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Link: http://lkml.kernel.org/r/20170924105913.9157-11-mingo@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/regset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index b831d5b..3ea1513 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -134,7 +134,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	fpu__prepare_write(fpu);
 
-	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+	if (using_compacted_format()) {
 		if (kbuf)
 			ret = copy_kernel_to_xstate(xsave, kbuf);
 		else

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


#1738144 — [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:10 +0200
Subject[PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utcXo-7Yz-9@gated-at.bofh.it>
In reply to#1738139
From: Eric Biggers <ebiggers@google.com>

Tighten the checks in sanitize_restored_xstate().

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/signal.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 1ef1b228b9fd..afe54247cf27 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -214,8 +214,11 @@ sanitize_restored_xstate(struct task_struct *tsk,
 	struct xstate_header *header = &xsave->header;
 
 	if (use_xsave()) {
-		/* These bits must be zero. */
-		memset(header->reserved, 0, 48);
+		/*
+		 * Note: we don't need to zero the reserved bits in the
+		 * xstate_header here because we either didn't copy them at all,
+		 * or we checked earlier that they aren't set.
+		 */
 
 		/*
 		 * Init the state that is not present in the memory
@@ -224,7 +227,7 @@ sanitize_restored_xstate(struct task_struct *tsk,
 		if (fx_only)
 			header->xfeatures = XFEATURE_MASK_FPSSE;
 		else
-			header->xfeatures &= (xfeatures_mask & xfeatures);
+			header->xfeatures &= xfeatures;
 	}
 
 	if (use_fxsr()) {
@@ -308,7 +311,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		/*
 		 * For 32-bit frames with fxstate, copy the user state to the
 		 * thread's fpu state, reconstruct fxstate from the fsave
-		 * header. Sanitize the copied state etc.
+		 * header. Validate and sanitize the copied state.
 		 */
 		struct fpu *fpu = &tsk->thread.fpu;
 		struct user_i387_ia32_struct env;
@@ -328,10 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		} else {
 			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
-
-			/* xcomp_bv must be 0 when using uncompacted format */
-			if (!err && fpu->state.xsave.header.xcomp_bv)
-				err = -EINVAL;
+			if (!err)
+				err = validate_xstate_header(&fpu->state.xsave.header);
 		}
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {
-- 
2.11.0

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


#1738250 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromEric Biggers <ebiggers3@gmail.com>
Date2017-09-24 21:00 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utkid-3NE-13@gated-at.bofh.it>
In reply to#1738144
On Sun, Sep 24, 2017 at 12:59:06PM +0200, Ingo Molnar wrote:
> @@ -328,10 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
>  		} else {
>  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> -
> -			/* xcomp_bv must be 0 when using uncompacted format */
> -			if (!err && fpu->state.xsave.header.xcomp_bv)
> -				err = -EINVAL;
> +			if (!err)
> +				err = validate_xstate_header(&fpu->state.xsave.header);
>  		}
>  

Sorry, this is the buggy part.  The problem is that this code runs even if XSAVE
isn't being used --- and in that case the state size is 512 bytes or less, so
the state doesn't actually include the xstate_header.  So
validate_xstate_header() was reading out of bounds and seeing invalid values.

So I think we need to check use_xsave() here, but it really needs to be in the
earlier patch which added the check for just ->xcomp_bv ("x86/fpu: Don't let
userspace set bogus xcomp_bv"), not in this one.

As far the split of patch 2/3 into these 10 patches, it looks fine (though it
suddenly became a *lot* of patches!).  One nit: the subject of this one really
should say "__fpu__restore_sig()", not "sanitize_restored_xstate()".

I can send a fixed series when I have a chance.

Eric

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


#1738252 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 21:10 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utkrU-46H-5@gated-at.bofh.it>
In reply to#1738250
* Eric Biggers <ebiggers3@gmail.com> wrote:

> On Sun, Sep 24, 2017 at 12:59:06PM +0200, Ingo Molnar wrote:
> > @@ -328,10 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> >  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> >  		} else {
> >  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> > -
> > -			/* xcomp_bv must be 0 when using uncompacted format */
> > -			if (!err && fpu->state.xsave.header.xcomp_bv)
> > -				err = -EINVAL;
> > +			if (!err)
> > +				err = validate_xstate_header(&fpu->state.xsave.header);
> >  		}
> >  
> 
> Sorry, this is the buggy part.  The problem is that this code runs even if XSAVE
> isn't being used --- and in that case the state size is 512 bytes or less, so
> the state doesn't actually include the xstate_header.  So
> validate_xstate_header() was reading out of bounds and seeing invalid values.
> 
> So I think we need to check use_xsave() here, but it really needs to be in the
> earlier patch which added the check for just ->xcomp_bv ("x86/fpu: Don't let
> userspace set bogus xcomp_bv"), not in this one.
> 
> As far the split of patch 2/3 into these 10 patches, it looks fine (though it
> suddenly became a *lot* of patches!).  One nit: the subject of this one really
> should say "__fpu__restore_sig()", not "sanitize_restored_xstate()".
> 
> I can send a fixed series when I have a chance.

Could you please just send the delta patch against the whole tree to fix the bug? 
I'll worry about the patch dependencies and back-merge it to the proper place.

Thanks,

	Ingo

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


#1738267 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromEric Biggers <ebiggers3@gmail.com>
Date2017-09-24 22:10 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utlnZ-4Gy-33@gated-at.bofh.it>
In reply to#1738252
On Sun, Sep 24, 2017 at 09:02:42PM +0200, Ingo Molnar wrote:
> 
> * Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > On Sun, Sep 24, 2017 at 12:59:06PM +0200, Ingo Molnar wrote:
> > > @@ -328,10 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> > >  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> > >  		} else {
> > >  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> > > -
> > > -			/* xcomp_bv must be 0 when using uncompacted format */
> > > -			if (!err && fpu->state.xsave.header.xcomp_bv)
> > > -				err = -EINVAL;
> > > +			if (!err)
> > > +				err = validate_xstate_header(&fpu->state.xsave.header);
> > >  		}
> > >  
> > 
> > Sorry, this is the buggy part.  The problem is that this code runs even if XSAVE
> > isn't being used --- and in that case the state size is 512 bytes or less, so
> > the state doesn't actually include the xstate_header.  So
> > validate_xstate_header() was reading out of bounds and seeing invalid values.
> > 
> > So I think we need to check use_xsave() here, but it really needs to be in the
> > earlier patch which added the check for just ->xcomp_bv ("x86/fpu: Don't let
> > userspace set bogus xcomp_bv"), not in this one.
> > 
> > As far the split of patch 2/3 into these 10 patches, it looks fine (though it
> > suddenly became a *lot* of patches!).  One nit: the subject of this one really
> > should say "__fpu__restore_sig()", not "sanitize_restored_xstate()".
> > 
> > I can send a fixed series when I have a chance.
> 
> Could you please just send the delta patch against the whole tree to fix the bug? 
> I'll worry about the patch dependencies and back-merge it to the proper place.
> 

The following diff against tip/master fixes the bug.  Note: we *could* check
'use_xsave()' instead of 'state_size > offsetof(struct xregs_state, header)',
but that might be confusing in the case where we couldn't find the xstate
information in the memory layout and only copy the fxregs_state, since then we'd
actually be validating the xsave_header which was already there, which shouldn't
ever fail.

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index afe54247cf27..fb639e70048f 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -331,7 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		} else {
 			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
-			if (!err)
+
+			if (!err && state_size > offsetof(struct xregs_state, header))
 				err = validate_xstate_header(&fpu->state.xsave.header);
 		}

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


#1738753 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-25 08:10 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utuKC-2qC-5@gated-at.bofh.it>
In reply to#1738267
* Eric Biggers <ebiggers3@gmail.com> wrote:

> The following diff against tip/master fixes the bug.  Note: we *could* check
> 'use_xsave()' instead of 'state_size > offsetof(struct xregs_state, header)',
> but that might be confusing in the case where we couldn't find the xstate
> information in the memory layout and only copy the fxregs_state, since then we'd
> actually be validating the xsave_header which was already there, which shouldn't
> ever fail.
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index afe54247cf27..fb639e70048f 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -331,7 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
>  		} else {
>  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> -			if (!err)
> +
> +			if (!err && state_size > offsetof(struct xregs_state, header))
>  				err = validate_xstate_header(&fpu->state.xsave.header);
>  		}

Yeah, I agree that checking 'state_size' is cleaner although note that technically 
this check isn't enough, because if 'state_size' is pointing somewhere inside the 
header (i.e. does not fully include it), the code still attempts a bad memcpy().

But that cannot happen, due to how state_size is set up:

        int state_size = fpu_kernel_xstate_size;

	...

                        state_size = sizeof(struct fxregs_state);
		...
                } else {
		...
                        state_size = fx_sw_user.xstate_size;
		...

and because fx_sw_user.xstate_size has to be at least:

        int min_xstate_size = sizeof(struct fxregs_state) +
                              sizeof(struct xstate_header);

i.e. the 'state_size' variable has a discrete set of possible values, none of 
which values point inside the header. Something to keep in mind ...


Note that there's some room for improvement within both the signal and the regset 
copying of FPU state. We have this pattern:

                if (using_compacted_format()) {
                        err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
                } else {
                        err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
                        if (!err)
                                err = validate_xstate_header(&fpu->state.xsave.header);
                }

... and copy_user_to_xstate() does:

        if (__copy_from_user(&hdr, ubuf + offset, size))
                return -EFAULT;

        if (validate_xstate_header(&hdr))
                return -EINVAL;

I.e. what we probably want is a helper function that just copies the darn thing 
and validates everything.

Note how regset.c duplicates a similar pattern:

        if (using_compacted_format()) {
                if (kbuf)
                        ret = copy_kernel_to_xstate(xsave, kbuf);
                else
                        ret = copy_user_to_xstate(xsave, ubuf);
        } else {
                ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
                if (!ret)
                        ret = validate_xstate_header(&xsave->header);
        }


I.e. what we should probably do is to push the using_compacted_format() check into 
copy_user_to_xstate(). That makes copy_user_to_xstate() a high level method that 
can deal with all formats and which does all verification.

But that's a separate cleanup.

Thanks,

	Ingo

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


#1738754 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-25 08:20 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utuUh-2ui-1@gated-at.bofh.it>
In reply to#1738267
* Eric Biggers <ebiggers3@gmail.com> wrote:

> On Sun, Sep 24, 2017 at 09:02:42PM +0200, Ingo Molnar wrote:
> > 
> > * Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > > On Sun, Sep 24, 2017 at 12:59:06PM +0200, Ingo Molnar wrote:
> > > > @@ -328,10 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> > > >  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> > > >  		} else {
> > > >  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> > > > -
> > > > -			/* xcomp_bv must be 0 when using uncompacted format */
> > > > -			if (!err && fpu->state.xsave.header.xcomp_bv)
> > > > -				err = -EINVAL;
> > > > +			if (!err)
> > > > +				err = validate_xstate_header(&fpu->state.xsave.header);
> > > >  		}
> > > >  
> > > 
> > > Sorry, this is the buggy part.  The problem is that this code runs even if XSAVE
> > > isn't being used --- and in that case the state size is 512 bytes or less, so
> > > the state doesn't actually include the xstate_header.  So
> > > validate_xstate_header() was reading out of bounds and seeing invalid values.
> > > 
> > > So I think we need to check use_xsave() here, but it really needs to be in the
> > > earlier patch which added the check for just ->xcomp_bv ("x86/fpu: Don't let
> > > userspace set bogus xcomp_bv"), not in this one.
> > > 
> > > As far the split of patch 2/3 into these 10 patches, it looks fine (though it
> > > suddenly became a *lot* of patches!).  One nit: the subject of this one really
> > > should say "__fpu__restore_sig()", not "sanitize_restored_xstate()".
> > > 
> > > I can send a fixed series when I have a chance.
> > 
> > Could you please just send the delta patch against the whole tree to fix the bug? 
> > I'll worry about the patch dependencies and back-merge it to the proper place.
> > 
> 
> The following diff against tip/master fixes the bug.  Note: we *could* check
> 'use_xsave()' instead of 'state_size > offsetof(struct xregs_state, header)',
> but that might be confusing in the case where we couldn't find the xstate
> information in the memory layout and only copy the fxregs_state, since then we'd
> actually be validating the xsave_header which was already there, which shouldn't
> ever fail.
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index afe54247cf27..fb639e70048f 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -331,7 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
>  		} else {
>  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> -			if (!err)
> +
> +			if (!err && state_size > offsetof(struct xregs_state, header))
>  				err = validate_xstate_header(&fpu->state.xsave.header);
>  		}

I.e. a better check would be to check that the whole header can be accessed:

	state_size >= offsetof(struct xregs_state, header) + sizeof(struct xstate_header)

Not that there should ever be a 'state_size' that points inside the header - so in 
the end I back-merged your original (and tested ...) version.

Thanks,

	Ingo

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


#1738783 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromEric Biggers <ebiggers3@gmail.com>
Date2017-09-25 09:30 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utw01-3bl-1@gated-at.bofh.it>
In reply to#1738754
On Mon, Sep 25, 2017 at 08:14:45AM +0200, Ingo Molnar wrote:
> > > 
> > > Could you please just send the delta patch against the whole tree to fix the bug? 
> > > I'll worry about the patch dependencies and back-merge it to the proper place.
> > > 
> > 
> > The following diff against tip/master fixes the bug.  Note: we *could* check
> > 'use_xsave()' instead of 'state_size > offsetof(struct xregs_state, header)',
> > but that might be confusing in the case where we couldn't find the xstate
> > information in the memory layout and only copy the fxregs_state, since then we'd
> > actually be validating the xsave_header which was already there, which shouldn't
> > ever fail.
> > 
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index afe54247cf27..fb639e70048f 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -331,7 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> >  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> >  		} else {
> >  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> > -			if (!err)
> > +
> > +			if (!err && state_size > offsetof(struct xregs_state, header))
> >  				err = validate_xstate_header(&fpu->state.xsave.header);
> >  		}
> 
> I.e. a better check would be to check that the whole header can be accessed:
> 
> 	state_size >= offsetof(struct xregs_state, header) + sizeof(struct xstate_header)
> 
> Not that there should ever be a 'state_size' that points inside the header - so in 
> the end I back-merged your original (and tested ...) version.
> 

Well, actually we'd need to validate the header if userspace overwrote any part
of it.

But more importantly, I think the state_size check needs to go into the first
patch (the one that's Cc'ed to stable as it fixes the real bug), since
->xcomp_bv is part of the xstate_header.  So *before* we switch to
validate_xstate_header() in this patch, the code should already be:

		if (using_compacted_format()) {
			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
		} else {
			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);

			/* xcomp_bv must be 0 when using uncompacted format */
			if (!err &&
			    state_size > offsetof(struct xregs_state, header) &&
			    fpu->state.xsave.header.xcomp_bv)
				err = -EINVAL;
		}

Also can you please fix the commit title and message of this patch?  It should
say "__fpu__restore_sig()", not "sanitize_restored_xstate()".

Thanks,

Eric

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


#1738793 — Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-25 09:40 +0200
SubjectRe: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Message-ID<utw9J-3eX-33@gated-at.bofh.it>
In reply to#1738783
* Eric Biggers <ebiggers3@gmail.com> wrote:

> On Mon, Sep 25, 2017 at 08:14:45AM +0200, Ingo Molnar wrote:
> > > > 
> > > > Could you please just send the delta patch against the whole tree to fix the bug? 
> > > > I'll worry about the patch dependencies and back-merge it to the proper place.
> > > > 
> > > 
> > > The following diff against tip/master fixes the bug.  Note: we *could* check
> > > 'use_xsave()' instead of 'state_size > offsetof(struct xregs_state, header)',
> > > but that might be confusing in the case where we couldn't find the xstate
> > > information in the memory layout and only copy the fxregs_state, since then we'd
> > > actually be validating the xsave_header which was already there, which shouldn't
> > > ever fail.
> > > 
> > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > > index afe54247cf27..fb639e70048f 100644
> > > --- a/arch/x86/kernel/fpu/signal.c
> > > +++ b/arch/x86/kernel/fpu/signal.c
> > > @@ -331,7 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> > >  			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> > >  		} else {
> > >  			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> > > -			if (!err)
> > > +
> > > +			if (!err && state_size > offsetof(struct xregs_state, header))
> > >  				err = validate_xstate_header(&fpu->state.xsave.header);
> > >  		}
> > 
> > I.e. a better check would be to check that the whole header can be accessed:
> > 
> > 	state_size >= offsetof(struct xregs_state, header) + sizeof(struct xstate_header)
> > 
> > Not that there should ever be a 'state_size' that points inside the header - so in 
> > the end I back-merged your original (and tested ...) version.
> > 
> 
> Well, actually we'd need to validate the header if userspace overwrote any part
> of it.
> 
> But more importantly, I think the state_size check needs to go into the first
> patch (the one that's Cc'ed to stable as it fixes the real bug), since
> ->xcomp_bv is part of the xstate_header.  So *before* we switch to
> validate_xstate_header() in this patch, the code should already be:
> 
> 		if (using_compacted_format()) {
> 			err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> 		} else {
> 			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> 
> 			/* xcomp_bv must be 0 when using uncompacted format */
> 			if (!err &&
> 			    state_size > offsetof(struct xregs_state, header) &&
> 			    fpu->state.xsave.header.xcomp_bv)
> 				err = -EINVAL;
> 		}

Note that I think the whole series is more robust if it goes to -stable as-is, as 
the ABI aspect should not be underestimated either. Nevertheless I've backmerged 
the fix further to the original commit, to maintain bisectability.

> Also can you please fix the commit title and message of this patch?  It should
> say "__fpu__restore_sig()", not "sanitize_restored_xstate()".

Indeed - and I fixed that too. I have pushed out the latest tip:WIP.x86/fpu - no 
change in the end result tree, but different inner structure.

Thanks,

	Ingo

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


#1739716 — [tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig()

Fromtip-bot for Eric Biggers <tipbot@zytor.com>
Date2017-09-26 10:50 +0200
Subject[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig()
Message-ID<utTJ1-2nL-49@gated-at.bofh.it>
In reply to#1738144
Commit-ID:  b11e2e18a7fc8eaa3d592c260d50c7129e094ded
Gitweb:     http://git.kernel.org/tip/b11e2e18a7fc8eaa3d592c260d50c7129e094ded
Author:     Eric Biggers <ebiggers@google.com>
AuthorDate: Sun, 24 Sep 2017 12:59:06 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 26 Sep 2017 09:43:46 +0200

x86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig()

Tighten the checks in __fpu__restore_sig() and update comments.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Link: http://lkml.kernel.org/r/20170924105913.9157-4-mingo@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/signal.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7fa3bdb..fb639e7 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -214,8 +214,11 @@ sanitize_restored_xstate(struct task_struct *tsk,
 	struct xstate_header *header = &xsave->header;
 
 	if (use_xsave()) {
-		/* These bits must be zero. */
-		memset(header->reserved, 0, 48);
+		/*
+		 * Note: we don't need to zero the reserved bits in the
+		 * xstate_header here because we either didn't copy them at all,
+		 * or we checked earlier that they aren't set.
+		 */
 
 		/*
 		 * Init the state that is not present in the memory
@@ -224,7 +227,7 @@ sanitize_restored_xstate(struct task_struct *tsk,
 		if (fx_only)
 			header->xfeatures = XFEATURE_MASK_FPSSE;
 		else
-			header->xfeatures &= (xfeatures_mask & xfeatures);
+			header->xfeatures &= xfeatures;
 	}
 
 	if (use_fxsr()) {
@@ -308,7 +311,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		/*
 		 * For 32-bit frames with fxstate, copy the user state to the
 		 * thread's fpu state, reconstruct fxstate from the fsave
-		 * header. Sanitize the copied state etc.
+		 * header. Validate and sanitize the copied state.
 		 */
 		struct fpu *fpu = &tsk->thread.fpu;
 		struct user_i387_ia32_struct env;
@@ -329,9 +332,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		} else {
 			err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
 
-			/* xcomp_bv must be 0 when using uncompacted format */
-			if (!err && state_size > offsetof(struct xregs_state, header) && fpu->state.xsave.header.xcomp_bv)
-				err = -EINVAL;
+			if (!err && state_size > offsetof(struct xregs_state, header))
+				err = validate_xstate_header(&fpu->state.xsave.header);
 		}
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {

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


#1738145 — [PATCH 07/10] x86/fpu: Copy the full header in copy_user_to_xstate()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:10 +0200
Subject[PATCH 07/10] x86/fpu: Copy the full header in copy_user_to_xstate()
Message-ID<utcXo-7Yz-11@gated-at.bofh.it>
In reply to#1738139
From: Eric Biggers <ebiggers@google.com>

This is in preparation to verify the full xstate header as supplied by user-space.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 325db7850335..0cd7b73c25e8 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1199,13 +1199,16 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	int i;
 	u64 xfeatures;
 	u64 allowed_features;
+	struct xstate_header hdr;
 
 	offset = offsetof(struct xregs_state, header);
-	size = sizeof(xfeatures);
+	size = sizeof(hdr);
 
-	if (__copy_from_user(&xfeatures, ubuf + offset, size))
+	if (__copy_from_user(&hdr, ubuf + offset, size))
 		return -EFAULT;
 
+	xfeatures = hdr.xfeatures;
+
 	/*
 	 * Reject if the user sets any disabled or supervisor features:
 	 */
-- 
2.11.0

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


#1739713 — [tip:x86/fpu] x86/fpu: Copy the full header in copy_user_to_xstate()

Fromtip-bot for Eric Biggers <tipbot@zytor.com>
Date2017-09-26 10:50 +0200
Subject[tip:x86/fpu] x86/fpu: Copy the full header in copy_user_to_xstate()
Message-ID<utTJ1-2nL-37@gated-at.bofh.it>
In reply to#1738145
Commit-ID:  af2c4322d986a08a6e793b74b83a62b325019c20
Gitweb:     http://git.kernel.org/tip/af2c4322d986a08a6e793b74b83a62b325019c20
Author:     Eric Biggers <ebiggers@google.com>
AuthorDate: Sun, 24 Sep 2017 12:59:10 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 26 Sep 2017 09:43:47 +0200

x86/fpu: Copy the full header in copy_user_to_xstate()

This is in preparation to verify the full xstate header as supplied by user-space.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Link: http://lkml.kernel.org/r/20170924105913.9157-8-mingo@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 325db78..0cd7b73 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1199,13 +1199,16 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	int i;
 	u64 xfeatures;
 	u64 allowed_features;
+	struct xstate_header hdr;
 
 	offset = offsetof(struct xregs_state, header);
-	size = sizeof(xfeatures);
+	size = sizeof(hdr);
 
-	if (__copy_from_user(&xfeatures, ubuf + offset, size))
+	if (__copy_from_user(&hdr, ubuf + offset, size))
 		return -EFAULT;
 
+	xfeatures = hdr.xfeatures;
+
 	/*
 	 * Reject if the user sets any disabled or supervisor features:
 	 */

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


#1738147 — [PATCH 01/10] x86/fpu: Introduce validate_xstate_header()

FromIngo Molnar <mingo@kernel.org>
Date2017-09-24 13:10 +0200
Subject[PATCH 01/10] x86/fpu: Introduce validate_xstate_header()
Message-ID<utcXo-7Yz-13@gated-at.bofh.it>
In reply to#1738139
From: Eric Biggers <ebiggers@google.com>

Move validation of user-supplied xstate_header into a helper function,
in preparation of calling it from both the ptrace and sigreturn syscall
paths.

The new function also considers it to be an error if *any* reserved bits
are set, whereas before we were just clearing most of them silently.

This should reduce the chance of bugs that fail to correctly validate
user-supplied XSAVE areas.  It also will expose any broken userspace
programs that set the other reserved bits; this is desirable because
such programs will lose compatibility with future CPUs and kernels if
those bits are ever used for anything.  (There shouldn't be any such
programs, and in fact in the case where the compacted format is in use
we were already validating xfeatures.  But you never know...)

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |  4 ++++
 arch/x86/kernel/fpu/xstate.c      | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 579ac2358e63..83fee2469eb7 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -52,4 +52,8 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
 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);
+
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+extern int validate_xstate_header(const struct xstate_header *hdr);
+
 #endif
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 703e76d027ee..2427aeea33b5 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -483,6 +483,30 @@ int using_compacted_format(void)
 	return boot_cpu_has(X86_FEATURE_XSAVES);
 }
 
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+int validate_xstate_header(const struct xstate_header *hdr)
+{
+	/* No unknown or supervisor features may be set */
+	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
+		return -EINVAL;
+
+	/* Userspace must use the uncompacted format */
+	if (hdr->xcomp_bv)
+		return -EINVAL;
+
+	/*
+	 * If 'reserved' is shrunken to add a new field, make sure to validate
+	 * that new field here!
+	 */
+	BUILD_BUG_ON(sizeof(hdr->reserved) != 48);
+
+	/* No reserved bits may be set */
+	if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
+		return -EINVAL;
+
+	return 0;
+}
+
 static void __xstate_dump_leaves(void)
 {
 	int i;
-- 
2.11.0

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


#1739714 — [tip:x86/fpu] x86/fpu: Introduce validate_xstate_header()

Fromtip-bot for Eric Biggers <tipbot@zytor.com>
Date2017-09-26 10:50 +0200
Subject[tip:x86/fpu] x86/fpu: Introduce validate_xstate_header()
Message-ID<utTJ1-2nL-39@gated-at.bofh.it>
In reply to#1738147
Commit-ID:  e63e5d5c15c6b1dba26f7cbd1b1089a1d6155db5
Gitweb:     http://git.kernel.org/tip/e63e5d5c15c6b1dba26f7cbd1b1089a1d6155db5
Author:     Eric Biggers <ebiggers@google.com>
AuthorDate: Sun, 24 Sep 2017 12:59:04 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 26 Sep 2017 09:43:45 +0200

x86/fpu: Introduce validate_xstate_header()

Move validation of user-supplied xstate_header into a helper function,
in preparation of calling it from both the ptrace and sigreturn syscall
paths.

The new function also considers it to be an error if *any* reserved bits
are set, whereas before we were just clearing most of them silently.

This should reduce the chance of bugs that fail to correctly validate
user-supplied XSAVE areas.  It also will expose any broken userspace
programs that set the other reserved bits; this is desirable because
such programs will lose compatibility with future CPUs and kernels if
those bits are ever used for anything.  (There shouldn't be any such
programs, and in fact in the case where the compacted format is in use
we were already validating xfeatures.  But you never know...)

Signed-off-by: Eric Biggers <ebiggers@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael Halcrow <mhalcrow@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: kernel-hardening@lists.openwall.com
Link: http://lkml.kernel.org/r/20170924105913.9157-2-mingo@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/xstate.h |  4 ++++
 arch/x86/kernel/fpu/xstate.c      | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 579ac23..83fee24 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -52,4 +52,8 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
 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);
+
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+extern int validate_xstate_header(const struct xstate_header *hdr);
+
 #endif
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 703e76d..2427aee 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -483,6 +483,30 @@ int using_compacted_format(void)
 	return boot_cpu_has(X86_FEATURE_XSAVES);
 }
 
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+int validate_xstate_header(const struct xstate_header *hdr)
+{
+	/* No unknown or supervisor features may be set */
+	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
+		return -EINVAL;
+
+	/* Userspace must use the uncompacted format */
+	if (hdr->xcomp_bv)
+		return -EINVAL;
+
+	/*
+	 * If 'reserved' is shrunken to add a new field, make sure to validate
+	 * that new field here!
+	 */
+	BUILD_BUG_ON(sizeof(hdr->reserved) != 48);
+
+	/* No reserved bits may be set */
+	if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
+		return -EINVAL;
+
+	return 0;
+}
+
 static void __xstate_dump_leaves(void)
 {
 	int i;

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


Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel


csiph-web