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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Cyrill Gorcunov <[email protected]>
Cc: Pankaj Bharadiya <[email protected]>
Cc: Aubrey Li <[email protected]>
Cc: [email protected]
Signed-off-by: Dave Hansen <[email protected]>
---
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,
_
On Wed, Jan 22, 2020 at 08:53:46AM -0800, Dave Hansen wrote:
>
> 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.
>
> @@ -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());
Should not we rather abort this operation and exit with EINVAL
or something similar instead of calling wrmsr with overflowed
value? IOW,
if (pkey >= arch_max_pkey()) {
WARN_ON_ONCE(1);
return -EINVAL;
}
On 1/22/20 10:51 AM, Cyrill Gorcunov wrote:
>> + /*
>> + * 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());
> Should not we rather abort this operation and exit with EINVAL
> or something similar instead of calling wrmsr with overflowed
> value? IOW,
>
> if (pkey >= arch_max_pkey()) {
> WARN_ON_ONCE(1);
> return -EINVAL;
> }
I don't feel strongly about it. The reason I didn't do that is to
minimize the chance that this would cause any functional regression.
It's not a huge chance, but I've certainly fat-fingered my share of
off-by-one bugs.
On Wed, Jan 22, 2020 at 11:09:47AM -0800, Dave Hansen wrote:
> On 1/22/20 10:51 AM, Cyrill Gorcunov wrote:
> >> + /*
> >> + * 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());
>
> > Should not we rather abort this operation and exit with EINVAL
> > or something similar instead of calling wrmsr with overflowed
> > value? IOW,
> >
> > if (pkey >= arch_max_pkey()) {
> > WARN_ON_ONCE(1);
> > return -EINVAL;
> > }
>
> I don't feel strongly about it. The reason I didn't do that is to
> minimize the chance that this would cause any functional regression.
OK, I don't mind leaving just WARN_ON_ONCE.
>
> It's not a huge chance, but I've certainly fat-fingered my share of
> off-by-one bugs.
Heh :)
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 16171bffc829272d5e6014bad48f680cb50943d9
Gitweb: https://git.kernel.org/tip/16171bffc829272d5e6014bad48f680cb50943d9
Author: Dave Hansen <[email protected]>
AuthorDate: Wed, 22 Jan 2020 08:53:46 -08:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Mon, 24 Feb 2020 20:25:21 +01:00
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 <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/pkeys.h | 5 +++++
arch/x86/kernel/fpu/xstate.c | 9 +++++++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 19b137f..2ff9b98 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -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,
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 73fe597..32b153d 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -895,8 +895,6 @@ const void *get_xsave_field_ptr(int xfeature_nr)
#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.
@@ -915,6 +913,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
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;