linux-kernel - [PATCH] x86/pkeys: add check for pkey "overflow"

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

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

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

Message-Id: <20200122165346.AD4DA150@viggo.jf.intel.com>
Date:   Wed, 22 Jan 2020 08:53:46 -0800
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     linux-kernel@...r.kernel.org
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        alex.shi@...ux.alibaba.com, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, hpa@...or.com, x86@...nel.org, bigeasy@...utronix.de,
        gorcunov@...il.com, pankaj.laxminarayan.bharadiya@...el.com,
        aubrey.li@...ux.intel.com, dave.hansen@...el.com
Subject: [PATCH] x86/pkeys: add check for pkey "overflow"


Alex Shi reported the pkey macros above arch_set_user_pkey_access()
to be unused.  They are unused, and even refer to a nonexistent
CONFIG option.

But, they might have served a good use, which was to ensure that
the code does not try to set values that would not fit in the
PKRU register.  As it stands, a too-large 'pkey' value would
be likely to silently overflow the u32 new_pkru_bits.

Add a check to look for overflows.  Also add a comment to remind
any future developer to closely examine the types used to store
pkey values if arch_max_pkey() ever changes.

This boots and passes the x86 pkey selftests.

Reported-by: Alex Shi <alex.shi@...ux.alibaba.com>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...hat.com>
Cc: Borislav Petkov <bp@...en8.de>
Cc: "H. Peter Anvin" <hpa@...or.com>
Cc: x86@...nel.org
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Cyrill Gorcunov <gorcunov@...il.com>
Cc: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@...el.com>
Cc: Aubrey Li <aubrey.li@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org
Signed-off-by: Dave Hansen <dave.hansen@...el.com>
---

 b/arch/x86/include/asm/pkeys.h |    5 +++++
 b/arch/x86/kernel/fpu/xstate.c |    9 +++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~pkey-check-pkru-shift arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~pkey-check-pkru-shift	2020-01-21 09:20:26.542385466 -0800
+++ b/arch/x86/kernel/fpu/xstate.c	2020-01-21 09:28:18.068384290 -0800
@@ -902,8 +902,6 @@ const void *get_xsave_field_ptr(int xfea
 
 #ifdef CONFIG_ARCH_HAS_PKEYS
 
-#define NR_VALID_PKRU_BITS (CONFIG_NR_PROTECTION_KEYS * 2)
-#define PKRU_VALID_MASK (NR_VALID_PKRU_BITS - 1)
 /*
  * This will go out and modify PKRU register to set the access
  * rights for @pkey to @init_val.
@@ -922,6 +920,13 @@ int arch_set_user_pkey_access(struct tas
 	if (!boot_cpu_has(X86_FEATURE_OSPKE))
 		return -EINVAL;
 
+	/*
+	 * This code should only be called with valid 'pkey'
+	 * values originating from in-kernel users.  Complain
+	 * if a bad value is observed.
+	 */
+	WARN_ON_ONCE(pkey >= arch_max_pkey());
+
 	/* Set the bits we need in PKRU:  */
 	if (init_val & PKEY_DISABLE_ACCESS)
 		new_pkru_bits |= PKRU_AD_BIT;
diff -puN arch/x86/include/asm/pkeys.h~pkey-check-pkru-shift arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkey-check-pkru-shift	2020-01-21 09:23:36.733384991 -0800
+++ b/arch/x86/include/asm/pkeys.h	2020-01-21 09:41:44.797382278 -0800
@@ -4,6 +4,11 @@
 
 #define ARCH_DEFAULT_PKEY	0
 
+/*
+ * If more than 16 keys are ever supported, a thorough audit
+ * will be necessary to ensure that the types that store key
+ * numbers and masks have sufficient capacity.
+ */
 #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
 
 extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
_