x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header"
cover x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header"
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(-)
arch/x86/include/asm/fpu/xstate.h +1 -22
@@ -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
arch/x86/kernel/fpu/xstate.c +26 -2
@@ -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++) {
PATCH 2/10 x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()
From: Eric Biggers <[email protected]>
Tighten the checks in xstateregs_set().
--- arch/x86/kernel/fpu/regset.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
arch/x86/kernel/fpu/regset.c +6 -13
@@ -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; }
Signature · 1 line
2.11.0
PATCH 5/10 x86/fpu: Eliminate the 'xfeatures' local variable in copy_kernel_to_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
We have this information in the xstate_header.
--- arch/x86/kernel/fpu/xstate.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
arch/x86/kernel/fpu/xstate.c +4 -6
@@ -1146,7 +1146,6 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) {unsigned int offset, size;int i; -u64 xfeatures;u64 allowed_features;struct xstate_header hdr;
@@ -1154,20 +1153,19 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) size =sizeof (hdr);memcpy (&hdr, kbuf + offset, size); - xfeatures = hdr.xfeatures;/* * Reject if the user sets any disabled or supervisor features: */ allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR ; -if (xfeatures & ~allowed_features) +if (hdr.xfeatures & ~allowed_features)return -EINVAL ;for (i =0 ; i <XFEATURE_MAX ; i++) {u64 mask = ((u64 )1 << i); -if (xfeatures & mask) { +if (hdr.xfeatures & mask) {void *dst =__raw_xsave_addr (xsave,1 << i); offset = xstate_offsets[i];
@@ -1177,7 +1175,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) } } -if (xfeatures_mxcsr_quirk (xfeatures)) { +if (xfeatures_mxcsr_quirk (hdr.xfeatures)) { offset = offsetof(struct fxregs_state , mxcsr); size =MXCSR_AND_FLAGS_SIZE ;memcpy (&xsave->i387.mxcsr, kbuf + offset, size);
@@ -1192,7 +1190,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)/* * Add back in the features that came in from userspace: */ - xsave->header.xfeatures |= xfeatures; + xsave->header.xfeatures |= hdr.xfeatures;return 0 ; }
Signature · 1 line
2.11.0
PATCH 9/10 x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
Tighten the checks in copy_user_to_xstate().
--- arch/x86/kernel/fpu/xstate.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
arch/x86/kernel/fpu/xstate.c +5 -11
@@ -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++) {
Signature · 1 line
2.11.0
PATCH 10/10 x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
--- arch/x86/kernel/fpu/regset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/fpu/regset.c +1 -1
@@ -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
Signature · 1 line
2.11.0
PATCH 8/10 x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
We now have this field in hdr.xfeatures.
--- arch/x86/kernel/fpu/xstate.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
arch/x86/kernel/fpu/xstate.c +4 -7
@@ -1197,7 +1197,6 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) {unsigned int offset, size;int i; -u64 xfeatures;u64 allowed_features;struct xstate_header hdr;
@@ -1207,20 +1206,18 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)if (__copy_from_user (&hdr, ubuf + offset, size))return -EFAULT ; - xfeatures = hdr.xfeatures; -/* * Reject if the user sets any disabled or supervisor features: */ allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR ; -if (xfeatures & ~allowed_features) +if (hdr.xfeatures & ~allowed_features)return -EINVAL ;for (i =0 ; i <XFEATURE_MAX ; i++) {u64 mask = ((u64 )1 << i); -if (xfeatures & mask) { +if (hdr.xfeatures & mask) {void *dst =__raw_xsave_addr (xsave,1 << i); offset = xstate_offsets[i];
@@ -1231,7 +1228,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) } } -if (xfeatures_mxcsr_quirk (xfeatures)) { +if (xfeatures_mxcsr_quirk (hdr.xfeatures)) { offset = offsetof(struct fxregs_state , mxcsr); size =MXCSR_AND_FLAGS_SIZE ;if (__copy_from_user (&xsave->i387.mxcsr, ubuf + offset, size))
@@ -1247,7 +1244,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)/* * Add back in the features that came in from userspace: */ - xsave->header.xfeatures |= xfeatures; + xsave->header.xfeatures |= hdr.xfeatures;return 0 ; }
Signature · 1 line
2.11.0
PATCH 7/10 x86/fpu: Copy the full header in copy_user_to_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
This is in preparation to verify the full xstate header as supplied by user-space.
--- arch/x86/kernel/fpu/xstate.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/xstate.c +5 -2
@@ -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: */
Signature · 1 line
2.11.0
PATCH 6/10 x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
Tighten the checks in copy_kernel_to_xstate().
--- arch/x86/kernel/fpu/xstate.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
arch/x86/kernel/fpu/xstate.c +2 -10
@@ -1138,15 +1138,12 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i/* * 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. +* and copy to the target thread. This is called from xstateregs_set(). */ int copy_kernel_to_xstate (struct xregs_state *xsave,const void *kbuf) {unsigned int offset, size;int i; -u64 allowed_features;struct xstate_header hdr; offset = offsetof(struct xregs_state , header);
@@ -1154,12 +1151,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)memcpy (&hdr, kbuf + offset, size); -/* -* 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++) {
Signature · 1 line
2.11.0
PATCH 4/10 x86/fpu: Copy the full state_header in copy_kernel_to_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
This is in preparation to verify the full xstate header as supplied by user-space.
--- arch/x86/kernel/fpu/xstate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/xstate.c +4 -2
@@ -1148,11 +1148,13 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)int i;u64 xfeatures;u64 allowed_features; +struct xstate_header hdr; offset = offsetof(struct xregs_state , header); - size =sizeof (xfeatures); + size =sizeof (hdr); -memcpy (&xfeatures, kbuf + offset, size); +memcpy (&hdr, kbuf + offset, size); + xfeatures = hdr.xfeatures;/* * Reject if the user sets any disabled or supervisor features:
Signature · 1 line
2.11.0
PATCH 3/10 x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
Tighten the checks in sanitize_restored_xstate().
--- arch/x86/kernel/fpu/signal.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
arch/x86/kernel/fpu/signal.c +9 -8
@@ -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))) {
Signature · 1 line
2.11.0
PATCH 1/10 x86/fpu: Introduce validate_xstate_header()
In reply to Ingo MolnarFrom: Eric Biggers <[email protected]>
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...)
--- arch/x86/include/asm/fpu/xstate.h | 4 ++++ arch/x86/kernel/fpu/xstate.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
arch/x86/include/asm/fpu/xstate.h +4 -0
@@ -52,4 +52,8 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int ofint 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
arch/x86/kernel/fpu/xstate.c +24 -0
@@ -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;
Signature · 1 line
2.11.0
Quoted 2 lines · Ingo Molnar
As mentioned before, the patch was too big and too complex, and I've split it up into 10 smaller, bisectable patches:
Is this the (updated) stuff you want to send in for 4.14? Because I like this smaller set more tor that than the big series I saw earlier.
I'm sure the bigger series is a better cleanup, but maybe we can wait with that bigger change for 4.15?
Linus
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
In reply to Ingo Molnararch/x86/kernel/fpu/signal.c static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 12 lines · Ingo Molnar
arch/x86/kernel/fpu/signal.c +2
-4@@ -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
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Ingo Molnar
As mentioned before, the patch was too big and too complex, and I've split it up into 10 smaller, bisectable patches:
Is this the (updated) stuff you want to send in for 4.14? Because I like this smaller set more tor that than the big series I saw earlier.
It's a 10 patch split-up of one of the 34 patches from the bigger series :-/
Quoted 2 lines
I'm sure the bigger series is a better cleanup, but maybe we can wait with that bigger change for 4.15?
So all of this is pretty unfortunate timing, caused in part because I delayed the FPU changes in this merge window due to having so many x86 changes already.
I'd really love to have the bigger series, firstly because beyond the simplifications and the fixes in the 10-patch series it also fixes some other problems such as this SkyLake bug:
0852b374173b: x86/fpu: Add FPU state copying quirk to handle XRSTOR failure on Intel Skylake CPUs
Secondly, with v4.14 being an LTS, doing the cleanups/simplifications afterwards adds a big backporting barrier.
But I can extract all the fixes and re-structure and re-test it all if you prefer it that way.
Thanks,
Ingo
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
In reply to Eric BiggersQuoted 23 lines · Eric Biggers
arch/x86/kernel/fpu/signal.c static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 12 lines · Ingo Molnar
arch/x86/kernel/fpu/signal.c +2
-4@@ -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. Sovalidate_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 bogusxcomp_bv"), not in this one.
Quoted 5 lines
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
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Quoted 26 lines · Ingo Molnar
Quoted 23 lines · Eric Biggers
Review static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 12 lines · Ingo Molnar
(quoted diff) +2
-4@@ -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. Sovalidate_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 bogusxcomp_bv"), not in this one.
Quoted 9 lines
Quoted 5 lines
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.
arch/x86/kernel/fpu/signal.c +2 -1
@@ -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); }
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Quoted 12 lines · Eric Biggers
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 thefxregs_state, since then we'd actually be validating thexsave_headerwhich was already there, which shouldn't ever fail.arch/x86/kernel/fpu/signal.c +0
-0
arch/x86/kernel/fpu/signal.c static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 8 lines
arch/x86/kernel/fpu/signal.c +2
-1@@ -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
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
In reply to Eric BiggersQuoted 49 lines · Eric Biggers
Quoted 26 lines · Ingo Molnar
Quoted 23 lines · Eric Biggers
Review static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 12 lines · Ingo Molnar
(quoted diff) +2
-4@@ -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. Sovalidate_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 bogusxcomp_bv"), not in this one.Quoted 9 lines
Quoted 5 lines
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 thefxregs_state, since then we'd actually be validating thexsave_headerwhich was already there, which shouldn't ever fail.arch/x86/kernel/fpu/signal.c +0
-0
arch/x86/kernel/fpu/signal.c static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 8 lines
arch/x86/kernel/fpu/signal.c +2
-1@@ -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
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Quoted 28 lines · Ingo Molnar
Quoted 17 lines
Quoted 4 lines
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 thefxregs_state, since then we'd actually be validating thexsave_headerwhich was already there, which shouldn't ever fail.arch/x86/kernel/fpu/signal.c +0
-0arch/x86/kernel/fpu/signal.c static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 8 lines
arch/x86/kernel/fpu/signal.c +2
-1@@ -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:
Quoted 5 lines
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
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()
Quoted 43 lines · Eric Biggers
Quoted 28 lines · Ingo Molnar
Quoted 17 lines
Quoted 4 lines
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 thefxregs_state, since then we'd actually be validating thexsave_headerwhich was already there, which shouldn't ever fail.arch/x86/kernel/fpu/signal.c +0
-0arch/x86/kernel/fpu/signal.c static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) · line 337
Quoted 8 lines
arch/x86/kernel/fpu/signal.c +2
-1@@ -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:
Quoted 5 lines
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_sizecheck 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 thexstate_header. So *before* we switch tovalidate_xstate_header()in this patch, the code should already be:
Quoted 11 lines
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.
Quoted 2 lines
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
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Introduce validate_xstate_header()
In reply to Ingo Molnarx86/fpu: Introduce validate_xstate_header()
Commit-ID: e63e5d5c15c6b1dba26f7cbd1b1089a1d6155db5 Gitweb: http://git.kernel.org/tip/e63e5d5c15c6b1dba26f7cbd1b1089a1d6155db5 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:04 +0200 Committer: Ingo Molnar <[email protected]> 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...)
--- arch/x86/include/asm/fpu/xstate.h | 4 ++++ arch/x86/kernel/fpu/xstate.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
arch/x86/include/asm/fpu/xstate.h +4 -0
@@ -52,4 +52,8 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int ofint 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
arch/x86/kernel/fpu/xstate.c +24 -0
@@ -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;
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()
In reply to Ingo Molnarx86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()
Commit-ID: cf9df81b139b6ebaec188d73758f02ca3b2110e4 Gitweb: http://git.kernel.org/tip/cf9df81b139b6ebaec188d73758f02ca3b2110e4 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:05 +0200 Committer: Ingo Molnar <[email protected]> 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().
--- arch/x86/kernel/fpu/regset.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
arch/x86/kernel/fpu/regset.c +6 -13
@@ -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; }
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig()
In reply to Ingo Molnarx86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig()
Commit-ID: b11e2e18a7fc8eaa3d592c260d50c7129e094ded Gitweb: http://git.kernel.org/tip/b11e2e18a7fc8eaa3d592c260d50c7129e094ded Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:06 +0200 Committer: Ingo Molnar <[email protected]> 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.
--- arch/x86/kernel/fpu/signal.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
arch/x86/kernel/fpu/signal.c +9 -7
@@ -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))) {
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate()
In reply to Ingo Molnarx86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate()
Commit-ID: af95774b3ca080b0e1e651c0fc7680f3444ddda7 Gitweb: http://git.kernel.org/tip/af95774b3ca080b0e1e651c0fc7680f3444ddda7 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:09 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 26 Sep 2017 09:43:47 +0200
x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate()
Tighten the checks in copy_kernel_to_xstate().
--- arch/x86/kernel/fpu/xstate.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
arch/x86/kernel/fpu/xstate.c +2 -10
@@ -1138,15 +1138,12 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i/* * 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. +* and copy to the target thread. This is called from xstateregs_set(). */ int copy_kernel_to_xstate (struct xregs_state *xsave,const void *kbuf) {unsigned int offset, size;int i; -u64 allowed_features;struct xstate_header hdr; offset = offsetof(struct xregs_state , header);
@@ -1154,12 +1151,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)memcpy (&hdr, kbuf + offset, size); -/* -* 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++) {
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Copy the full header in copy_user_to_xstate()
In reply to Ingo Molnarx86/fpu: Copy the full header in copy_user_to_xstate()
Commit-ID: af2c4322d986a08a6e793b74b83a62b325019c20 Gitweb: http://git.kernel.org/tip/af2c4322d986a08a6e793b74b83a62b325019c20 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:10 +0200 Committer: Ingo Molnar <[email protected]> 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.
--- arch/x86/kernel/fpu/xstate.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/xstate.c +5 -2
@@ -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: */
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Eliminate the 'xfeatures' local variable in copy_kernel_to_xstate()
In reply to Ingo Molnarx86/fpu: Eliminate the 'xfeatures' local variable in copy_kernel_to_xstate()
Commit-ID: b89eda482d7849a1c146b6d0a42f4e76369bb08e Gitweb: http://git.kernel.org/tip/b89eda482d7849a1c146b6d0a42f4e76369bb08e Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:08 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 26 Sep 2017 09:43:46 +0200
x86/fpu: Eliminate the 'xfeatures' local variable in copy_kernel_to_xstate()
We have this information in the xstate_header.
--- arch/x86/kernel/fpu/xstate.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
arch/x86/kernel/fpu/xstate.c +4 -6
@@ -1146,7 +1146,6 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) {unsigned int offset, size;int i; -u64 xfeatures;u64 allowed_features;struct xstate_header hdr;
@@ -1154,20 +1153,19 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) size =sizeof (hdr);memcpy (&hdr, kbuf + offset, size); - xfeatures = hdr.xfeatures;/* * Reject if the user sets any disabled or supervisor features: */ allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR ; -if (xfeatures & ~allowed_features) +if (hdr.xfeatures & ~allowed_features)return -EINVAL ;for (i =0 ; i <XFEATURE_MAX ; i++) {u64 mask = ((u64 )1 << i); -if (xfeatures & mask) { +if (hdr.xfeatures & mask) {void *dst =__raw_xsave_addr (xsave,1 << i); offset = xstate_offsets[i];
@@ -1177,7 +1175,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) } } -if (xfeatures_mxcsr_quirk (xfeatures)) { +if (xfeatures_mxcsr_quirk (hdr.xfeatures)) { offset = offsetof(struct fxregs_state , mxcsr); size =MXCSR_AND_FLAGS_SIZE ;memcpy (&xsave->i387.mxcsr, kbuf + offset, size);
@@ -1192,7 +1190,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)/* * Add back in the features that came in from userspace: */ - xsave->header.xfeatures |= xfeatures; + xsave->header.xfeatures |= hdr.xfeatures;return 0 ; }
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()
In reply to Ingo Molnarx86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate()
Commit-ID: 98c0fad9d60e8b2cd47e15b7bee7df343648f5bb Gitweb: http://git.kernel.org/tip/98c0fad9d60e8b2cd47e15b7bee7df343648f5bb Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:12 +0200 Committer: Ingo Molnar <[email protected]> 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().
--- arch/x86/kernel/fpu/xstate.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
arch/x86/kernel/fpu/xstate.c +5 -11
@@ -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++) {
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES
In reply to Ingo Molnarx86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES
Commit-ID: 738f48cb5fdd5878d11934f1898aa2bcf1578289 Gitweb: http://git.kernel.org/tip/738f48cb5fdd5878d11934f1898aa2bcf1578289 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:13 +0200 Committer: Ingo Molnar <[email protected]> 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.
--- arch/x86/kernel/fpu/regset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/fpu/regset.c +1 -1
@@ -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
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate()
In reply to Ingo Molnarx86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate()
Commit-ID: 3d703477bcfe8bb57079d97198cf1e342fe1fef9 Gitweb: http://git.kernel.org/tip/3d703477bcfe8bb57079d97198cf1e342fe1fef9 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:11 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 26 Sep 2017 09:43:48 +0200
x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate()
We now have this field in hdr.xfeatures.
--- arch/x86/kernel/fpu/xstate.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
arch/x86/kernel/fpu/xstate.c +4 -7
@@ -1197,7 +1197,6 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) {unsigned int offset, size;int i; -u64 xfeatures;u64 allowed_features;struct xstate_header hdr;
@@ -1207,20 +1206,18 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)if (__copy_from_user (&hdr, ubuf + offset, size))return -EFAULT ; - xfeatures = hdr.xfeatures; -/* * Reject if the user sets any disabled or supervisor features: */ allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR ; -if (xfeatures & ~allowed_features) +if (hdr.xfeatures & ~allowed_features)return -EINVAL ;for (i =0 ; i <XFEATURE_MAX ; i++) {u64 mask = ((u64 )1 << i); -if (xfeatures & mask) { +if (hdr.xfeatures & mask) {void *dst =__raw_xsave_addr (xsave,1 << i); offset = xstate_offsets[i];
@@ -1231,7 +1228,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) } } -if (xfeatures_mxcsr_quirk (xfeatures)) { +if (xfeatures_mxcsr_quirk (hdr.xfeatures)) { offset = offsetof(struct fxregs_state , mxcsr); size =MXCSR_AND_FLAGS_SIZE ;if (__copy_from_user (&xsave->i387.mxcsr, ubuf + offset, size))
@@ -1247,7 +1244,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)/* * Add back in the features that came in from userspace: */ - xsave->header.xfeatures |= xfeatures; + xsave->header.xfeatures |= hdr.xfeatures;return 0 ; }
tip-bot for Eric Biggers
merge bot[tip:x86/fpu] x86/fpu: Copy the full state_header in copy_kernel_to_xstate()
In reply to Ingo Molnarx86/fpu: Copy the full state_header in copy_kernel_to_xstate()
Commit-ID: 80d8ae86b36791a545ca28ddc95133ea59bba6e0 Gitweb: http://git.kernel.org/tip/80d8ae86b36791a545ca28ddc95133ea59bba6e0 Author: Eric Biggers <[email protected]> AuthorDate: Sun, 24 Sep 2017 12:59:07 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 26 Sep 2017 09:43:46 +0200
x86/fpu: Copy the full state_header in copy_kernel_to_xstate()
This is in preparation to verify the full xstate header as supplied by user-space.
--- arch/x86/kernel/fpu/xstate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/xstate.c +4 -2
@@ -1148,11 +1148,13 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)int i;u64 xfeatures;u64 allowed_features; +struct xstate_header hdr; offset = offsetof(struct xregs_state , header); - size =sizeof (xfeatures); + size =sizeof (hdr); -memcpy (&xfeatures, kbuf + offset, size); +memcpy (&hdr, kbuf + offset, size); + xfeatures = hdr.xfeatures;/* * Reject if the user sets any disabled or supervisor features:
[RFC GIT PULL] x86 FPU fixes and cleanups
In reply to Ingo MolnarQuoted 10 lines · Ingo Molnar
Quoted 2 lines
I'm sure the bigger series is a better cleanup, but maybe we can wait with that bigger change for 4.15?
So all of this is pretty unfortunate timing, caused in part because I delayed the FPU changes in this merge window due to having so many x86 changes already.
I'd really love to have the bigger series, firstly because beyond the simplifications and the fixes in the 10-patch series it also fixes some other problems such as this SkyLake bug:
Quoted 7 lines
0852b374173b: x86/fpu: Add FPU state copying quirk to handle XRSTOR failure on Intel Skylake CPUsSecondly, with v4.14 being an LTS, doing the cleanups/simplifications afterwards adds a big backporting barrier.
But I can extract all the fixes and re-structure and re-test it all if you prefer it that way.
So this an RFC pull request, you can pull the latest x86-fpu-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86-fpu-for-linus
# HEAD: 8474c532b558fb6754c0ddff42a39ed6636671c9 Merge branch 'WIP.x86/fpu' into x86/fpu, because it's ready
This includes the recent XSAVE handling fixes from Eric Biggers (with all known bugs in it fixed), and the SkyLake XRSTOR handling fix from Rik van Riel, plus a number of cleanups/simplifications that addressed a number of more structural problems that enabled those bugs.
I have tested it on a range of x86 systems, new and old alike, and tested a number of corner cases as well - but as you can see it from the Git tree it got rebased recent to stay bisectable.
Due to v4.14 being an LTS release the restructuring would be a pretty big backporting barrier, but the tree is large and this was intended for v4.15 until the latest rounds of fixes showed up - so the timing is unfortunate and this pull request is late.
If you cannot take this then I'll extract the fixes and redo the tree and send you another pull request in a couple of days.
Thanks,
Ingo
------------------>
Andi Kleen (1):
x86/fpu: Turn WARN_ON() in context switch into WARN_ON_FPU()
Eric Biggers (12):
x86/fpu: Don't let userspace set bogus xcomp_bv
x86/fpu: Reinitialize FPU registers if restoring FPU state fails
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 __fpu__restore_sig() 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
Ingo Molnar (27):
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()
x86/fpu: Simplify fpu->fpregs_active use
x86/fpu: Make the fpu state change in fpu__clear() scheduler-atomic
x86/fpu: Split the state handling in fpu__drop()
x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active
x86/fpu: Decouple fpregs_activate()/fpregs_deactivate() from fpu->fpregs_active x86/fpu: Remove struct fpu::fpregs_active
x86/fpu: Fix fpu__activate_fpstate_read() and update comments
x86/fpu: Remove fpu__current_fpstate_write_begin/end()
x86/fpu: Rename fpu::fpstate_active to fpu::initialized
x86/fpu: Fix stale comments about lazy FPU logic
x86/fpu: Simplify and speed up fpu__copy()
x86/fpu: Rename fpu__activate_curr() to fpu__initialize()
x86/fpu: Rename fpu__activate_fpstate_read/write() to fpu__prepare_[read|write]()
Rik van Riel (1):
x86/fpu: Add FPU state copying quirk to handle XRSTOR failure on Intel Skylake CPUs
kbuild test robot (1):
x86/fpu: Fix boolreturn.cocci warnings
arch/x86/ia32/ia32_signal.c | 2 +- arch/x86/include/asm/fpu/internal.h | 90 +++--------- arch/x86/include/asm/fpu/types.h | 32 +---- arch/x86/include/asm/fpu/xstate.h | 12 +- arch/x86/include/asm/trace/fpu.h | 11 +- arch/x86/kernel/fpu/core.c | 155 ++++++--------------- arch/x86/kernel/fpu/init.c | 2 +- arch/x86/kernel/fpu/regset.c | 48 ++++--- arch/x86/kernel/fpu/signal.c | 37 ++--- arch/x86/kernel/fpu/xstate.c | 264 +++++++++++++++++++++++++++++------- arch/x86/kernel/signal.c | 6 +- arch/x86/kvm/x86.c | 2 +- arch/x86/math-emu/fpu_entry.c | 2 +- arch/x86/mm/extable.c | 24 ++++ arch/x86/mm/pkeys.c | 3 +- 15 files changed, 375 insertions(+), 315 deletions(-)
arch/x86/ia32/ia32_signal.c +1 -1
@@ -231,7 +231,7 @@ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs, ksig->ka.sa.sa_restorer) sp = (unsigned long ) ksig->ka.sa.sa_restorer; -if (fpu->fpstate_active) { +if (fpu->initialized) {unsigned long fx_aligned, math_size; sp =fpu__alloc_mathframe (sp,1 , &fx_aligned, &math_size);
arch/x86/include/asm/fpu/internal.h +22 -68
193 lines not on this page · raw message
arch/x86/include/asm/fpu/types.h +6 -26
@@ -68,6 +68,9 @@ struct fxregs_state {/* Default value for fxregs_state.mxcsr: */ #define MXCSR_DEFAULT 0x1f80 +/* Copy both mxcsr & mxcsr_flags with a single u64 memcpy: */ +#define MXCSR_AND_FLAGS_SIZE sizeof(u64) +/* * Software based FPU emulation state. This is arbitrary really, * it matches the x87 format to make it easier to understand:
@@ -290,36 +293,13 @@ struct fpu {unsigned int last_cpu;/* -* @fpstate_active: +* @initialized: * -* This flag indicates whether this context is active: if the task +* This flag indicates whether this context is initialized: if the task * is not running then we can restore from this context, if the task * is running then we should save into this context. */ -unsigned char fpstate_active; - -/* -* @fpregs_active: -* -* This flag determines whether a given context is actively -* loaded into the FPU's registers and that those registers -* represent the task's current FPU state. -* -* Note the interaction with fpstate_active: -* -* # task does not use the FPU: -* fpstate_active == 0 -* -* # task uses the FPU and regs are active: -* fpstate_active == 1 && fpregs_active == 1 -* -* # the regs are inactive but still match fpstate: -* fpstate_active == 1 && fpregs_active == 0 && fpregs_owner == fpu -* -* The third state is what we use for the lazy restore optimization -* on lazy-switching CPUs. -*/ -unsigned char fpregs_active; +unsigned char initialized;/* * @state:
arch/x86/include/asm/fpu/xstate.h +8 -4
@@ -48,8 +48,12 @@ 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 copyout_from_xsaves (unsigned int pos,unsigned int count,void *kbuf, -void __user *ubuf,struct xregs_state *xsave); -int copyin_to_xsaves (const void *kbuf,const void __user *ubuf, -struct xregs_state *xsave); +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 (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
arch/x86/include/asm/trace/fpu.h +4 -7
@@ -12,25 +12,22 @@ DECLARE_EVENT_CLASS(x86_fpu,TP_STRUCT__entry (__field (struct fpu *,fpu ) -__field (bool, fpregs_active) -__field (bool, fpstate_active) +__field (bool, initialized)__field (u64, xfeatures)__field (u64, xcomp_bv) ),TP_fast_assign ( __entry->fpu = fpu; - __entry->fpregs_active = fpu->fpregs_active; - __entry->fpstate_active = fpu->fpstate_active; + __entry->initialized = fpu->initialized;if (boot_cpu_has (X86_FEATURE_OSXSAVE )) { __entry->xfeatures = fpu->state.xsave.header.xfeatures; __entry->xcomp_bv = fpu->state.xsave.header.xcomp_bv; } ), -TP_printk ("x86/fpu: %p fpregs_active: %d fpstate_active: %d xfeatures: %llx xcomp_bv: %llx" , +TP_printk ("x86/fpu: %p initialized: %d xfeatures: %llx xcomp_bv: %llx" , __entry->fpu, - __entry->fpregs_active, - __entry->fpstate_active, + __entry->initialized, __entry->xfeatures, __entry->xcomp_bv )
arch/x86/kernel/fpu/core.c +43 -112
275 lines not on this page · raw message
arch/x86/kernel/fpu/init.c +1 -1
@@ -240,7 +240,7 @@ static void __init fpu__init_system_ctx_switch(void)WARN_ON_FPU (!on_boot_cpu); on_boot_cpu =0 ; -WARN_ON_FPU (current->thread.fpu.fpstate_active); +WARN_ON_FPU (current->thread.fpu.initialized); }/*
arch/x86/kernel/fpu/regset.c +26 -22
113 lines not on this page · raw message
arch/x86/kernel/fpu/signal.c +21 -16
@@ -155,7 +155,8 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)*/ int copy_fpstate_to_sigframe (void __user *buf,void __user *buf_fx,int size) { -struct xregs_state *xsave = ¤t->thread.fpu.state.xsave; +struct fpu *fpu = ¤t->thread.fpu; +struct xregs_state *xsave = &fpu->state.xsave;struct task_struct *tsk = current;int ia32_fxstate = (buf != buf_fx);
@@ -170,13 +171,13 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)sizeof (struct user_i387_ia32_struct ),NULL , (struct _fpstate_32 __user *) buf) ? -1 : 1; -if (fpregs_active () ||using_compacted_format ()) { +if (fpu->initialized ||using_compacted_format ()) {/* Save the live register state to the user directly. */ if (copy_fpregs_to_sigframe (buf_fx))return -1 ;/* Update the thread's fxstate to save the fsave header. */ if (ia32_fxstate) -copy_fxregs_to_kernel (&tsk->thread.fpu); +copy_fxregs_to_kernel (fpu); }else {/* * It is a *bug* if kernel uses compacted-format for xsave
@@ -189,7 +190,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)return -1 ; } -fpstate_sanitize_xstate (&tsk->thread.fpu); +fpstate_sanitize_xstate (fpu);if (__copy_to_user (buf_fx, xsave, fpu_user_xstate_size))return -1 ; }
@@ -213,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
@@ -223,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 ()) {
@@ -279,7 +283,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)if (!access_ok (VERIFY_READ , buf, size))return -EACCES ; -fpu__activate_curr (fpu); +fpu__initialize (fpu);if (!static_cpu_has (X86_FEATURE_FPU )) return fpregs_soft_set(current, NULL,
@@ -307,28 +311,29 @@ 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;int err =0 ;/* -* Drop the current fpu which clears fpu->fpstate_active. This ensures +* Drop the current fpu which clears fpu->initialized. This ensures * that any context-switch during the copy of the new state, * avoids the intermediate state from getting restored/saved. * Thus avoiding the new restored state from getting corrupted. * We will be ready to restore/save the state only after -* fpu->fpstate_active is again set. +* fpu->initialized is again set. */ fpu__drop (fpu);if (using_compacted_format ()) { - err =copyin_to_xsaves (NULL , 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); + err =__copy_from_user (&fpu->state.xsave, buf_fx, state_size); + +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))) {
@@ -339,7 +344,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)sanitize_restored_xstate (tsk, &env, xfeatures, fx_only); } - fpu->fpstate_active =1 ; + fpu->initialized =1 ;preempt_disable ();fpu__restore (fpu);preempt_enable ();
arch/x86/kernel/fpu/xstate.c +213 -51
366 lines not on this page · raw message
arch/x86/kernel/signal.c +3 -3
@@ -263,7 +263,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size, sp = (unsigned long ) ka->sa.sa_restorer; } -if (fpu->fpstate_active) { +if (fpu->initialized) { sp =fpu__alloc_mathframe (sp,IS_ENABLED (CONFIG_X86_32 ), &buf_fx, &math_size); *fpstate = (void __user *)sp;
@@ -279,7 +279,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,return (void __user *)-1L ;/* save i387 and extended state */ -if (fpu->fpstate_active && +if (fpu->initialized &©_fpstate_to_sigframe (*fpstate, (void __user *)buf_fx, math_size) <0 )return (void __user *)-1L ;
@@ -755,7 +755,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)/* * Ensure the signal handler starts with the new fpu state. */ -if (fpu->fpstate_active) +if (fpu->initialized)fpu__clear (fpu); }signal_setup_done (failed, ksig, stepping);
arch/x86/kvm/x86.c +1 -1
@@ -7225,7 +7225,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)int r;sigset_t sigsaved; -fpu__activate_curr (fpu); +fpu__initialize (fpu);if (vcpu->sigset_active)sigprocmask (SIG_SETMASK , &vcpu->sigset, &sigsaved);
arch/x86/math-emu/fpu_entry.c +1 -1
@@ -114,7 +114,7 @@ void math_emulate(struct math_emu_info *info)struct desc_struct code_descriptor;struct fpu *fpu = ¤t->thread.fpu; -fpu__activate_curr (fpu); +fpu__initialize (fpu);#ifdef RE_ENTRANT_CHECKING if (emulating) {
arch/x86/mm/extable.c +24 -0
@@ -2,6 +2,7 @@#include <linux/uaccess.h> #include <linux/sched/debug.h> +#include <asm/fpu/internal.h> #include <asm/traps.h> #include <asm/kdebug.h>
@@ -78,6 +79,29 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup, }EXPORT_SYMBOL_GPL (ex_handler_refcount); +/* +* Handler for when we fail to restore a task's FPU state. We should never get +* here because the FPU state of a task using the FPU (task->thread.fpu.state) +* should always be valid. However, past bugs have allowed userspace to set +* reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn(). +* These caused XRSTOR to fail when switching to the task, leaking the FPU +* registers of the task previously executing on the CPU. Mitigate this class +* of vulnerability by restoring from the initial state (essentially, zeroing +* out all the FPU registers) if we can't restore from the task's FPU state. +*/ +bool ex_handler_fprestore (const struct exception_table_entry *fixup, +struct pt_regs *regs,int trapnr) +{ + regs->ip =ex_fixup_addr (fixup); + +WARN_ONCE (1 ,"Bad FPU state detected at %pB, reinitializing FPU registers." , + (void *)instruction_pointer (regs)); + +__copy_kernel_to_fpregs (&init_fpstate,-1 ); +return true; +} +EXPORT_SYMBOL_GPL (ex_handler_fprestore); +bool ex_handler_ext (const struct exception_table_entry *fixup,struct pt_regs *regs,int trapnr) {
arch/x86/mm/pkeys.c +1 -2
@@ -18,7 +18,6 @@#include <asm/cpufeature.h> /* boot_cpu_has, ... */ #include <asm/mmu_context.h> /* vma_pkey() */ -#include <asm/fpu/internal.h> /* fpregs_active() */ int __execute_only_pkey (struct mm_struct *mm) {
@@ -45,7 +44,7 @@ int __execute_only_pkey(struct mm_struct *mm)*/ preempt_disable ();if (!need_to_set_mm_pkey && -fpregs_active () && + current->thread.fpu.initialized && !__pkru_allows_read (read_pkru (), execute_only_pkey)) {preempt_enable ();return execute_only_pkey;
Re: [RFC GIT PULL] x86 FPU fixes and cleanups
Quoted 3 lines · Ingo Molnar
So this an RFC pull request, you can pull the latest x86-fpu-for-linus git tree from:
Ugh.
Ok, I think I'll do this, but I'd ask that instead of that final merge commit, you'll just send me the series as normal, and with a _extensive_ explanation that I can then put in my merge message about why this is all a good idea at this time, and what is going on.
How does that sound?
Linus
[RFC GIT PULL, v2] x86 FPU fixes and cleanups
Quoted 5 lines · Linus Torvalds
Quoted 3 lines · Ingo Molnar
So this an RFC pull request, you can pull the latest x86-fpu-for-linus git tree from:
Quoted 8 lines
Ugh.
Ok, I think I'll do this, but I'd ask that instead of that final merge commit, you'll just send me the series as normal, and with a _extensive_ explanation that I can then put in my merge message about why this is all a good idea at this time, and what is going on.
How does that sound?
Ok!
Here's the updated pull request that is pointing to the linear series of commits with no merge commits, plus (much...) more information about what it all does:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86-fpu-for-linus
# HEAD: 8474c532b558fb6754c0ddff42a39ed6636671c9 Merge branch 'WIP.x86/fpu' into x86/fpu, because it's ready
Here's a list of all the commits, in mostly chronological/topical order (i.e. it's mostly reverse git log order, with a bit of topical re-grouping which are marked with '...'):
These 14 commits:
656f083116a4: x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user()f0d4f30a7fd2: x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user() 4d981cf2d96f: x86/fpu: Remove 'ubuf' parameter from the copy_xstate_to_kernel() APIsa69c158fb3e7: x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIsd7eda6c99cc7: x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIsbecb2bb72ff9: x86/fpu: Clean up the parameter definitions of copy_xstate_to_*()8a5b731889cb: x86/fpu: Remove the 'start_pos' parameter from the __copy_xstate_to_*() functions 56583c9a1400: x86/fpu: Clarify parameter names in the copy_xstate_to_*() methods6ff15f8db7ea: x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*() 8c0817f4a318: x86/fpu: Simplify __copy_xstate_to_kernel() return values79fecc2b7506: x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate() 59dffa4edba1: x86/fpu: Remove 'ubuf' parameter from the copy_kernel_to_xstate() API7b9094c688f8: x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API6d7f7da5533a: x86/fpu: Flip the parameter order in copy_*_to_xstate()
... clean up and simplify the xstate user copy handling functions, in reaction to the collective head-scratching about the xstate user-copy handling code when this SkyLake xstate handling bug was fixed:
0852b374173b: x86/fpu: Add FPU state copying quirk to handle XRSTOR failure on Intel Skylake CPUs
The cleanups don't change any functionality, they just (hopefully) make it all clearer, more consistent, more debuggable and more robust. Note that most of the linecount increase comes from these commits, where we better split the user/kernel copy logic by having more variants, instead repeated fragile patterns of:
+ if (kbuf) {
+ memcpy(kbuf + pos, data, copy);
+ } else {
+ if (__copy_to_user(ubuf + pos, data, copy))
+ return -EFAULT;
+ }
These 13 commits:
b3a163081c28: x86/fpu: Simplify fpu->fpregs_active usea10b6a16cdad: x86/fpu: Make the fpu state change in fpu__clear() scheduler-atomicb6aa85558d7e: x86/fpu: Split the state handling in fpu__drop()f1c8cd017607: x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active 6cf4edbe0526: x86/fpu: Decouple fpregs_activate()/fpregs_deactivate() from fpu->fpregs_active 99dc26bda233: x86/fpu: Remove struct fpu::fpregs_active
...4618e90965f2: x86/fpu: Fix fpu__activate_fpstate_read() and update comments 685c930d6e58: x86/fpu: Remove fpu__current_fpstate_write_begin/end()e4a81bfcaae1: x86/fpu: Rename fpu::fpstate_active to fpu::initialized 7f1487c59b7c: x86/fpu: Fix stale comments about lazy FPU logice10078eba698: x86/fpu: Simplify and speed up fpu__copy()2ce03d850b9a: x86/fpu: Rename fpu__activate_curr() to fpu__initialize() 369a036de206: x86/fpu: Rename fpu__activate_fpstate_read/write() to fpu__prepare_[read|write]()
... simplify the FPU state-machine to get rid of old lazy-FPU idiosyncrasies - a defensive simplification to make all the code easier to review and fix. No change in functionality.
These two commits:
4f8cef59bad2: x86/fpu: Fix boolreturn.cocci warnings 03eaec81ac09: x86/fpu: Turn WARN_ON() in context switch into WARN_ON_FPU()
... are debugging tweaks: static checker warning fix and move an FPU related warning to under WARN_ON_FPU().
These 12 commits:
814fb7bb7db5: x86/fpu: Don't let userspace set bogus xcomp_bv
...d5c8028b4788: x86/fpu: Reinitialize FPU registers if restoring FPU state failse63e5d5c15c6: x86/fpu: Introduce validate_xstate_header()cf9df81b139b: x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set()b11e2e18a7fc: x86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig() 80d8ae86b367: x86/fpu: Copy the full state_header in copy_kernel_to_xstate()b89eda482d78: x86/fpu: Eliminate the 'xfeatures' local variable in copy_kernel_to_xstate()af95774b3ca0: x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate()af2c4322d986: x86/fpu: Copy the full header in copy_user_to_xstate()3d703477bcfe: x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate() 98c0fad9d60e: x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate() 738f48cb5fdd: x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES
... represent a finegrained split-up of the fixes from Eric Biggers to handle weird xstate bits properly. I did this finegrained split-up because some of these fixes also impact the ABI for weird xstate handling, for which we'd like to have good bisection results, should they cause any problems. (We also had one regression with the more monolithic fixes, so splitting it all up sounded prudent for robustness reasons as well.)
About the whole series: the commits up to 03eaec81ac09 have been in -next for months - but I've recently rebased them to remove a state machine clean-up commit that was objected to, and to make it more bisectable - so technically it's a new, rebased tree.
Robustness history: this series had some regressions along the way, and all reported regressions have been fixed. All but one of the regressions manifested itself as easy to report warnings. The previous version of this latest series was also in linux-next, with one (warning-only) regression reported which is fixed in the latest version.
Barring last minute brown paper bag bugs (and the commits are now older by a day which I'd hope helps paperbag reduction), I'm reasonably confident about its general robustness. Famous last words ...
Thanks,
Ingo
------------------>
Andi Kleen (1):
x86/fpu: Turn WARN_ON() in context switch into WARN_ON_FPU()
Eric Biggers (12):
x86/fpu: Don't let userspace set bogus xcomp_bv
x86/fpu: Reinitialize FPU registers if restoring FPU state fails
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 __fpu__restore_sig() 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
Ingo Molnar (27):
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()
x86/fpu: Simplify fpu->fpregs_active use
x86/fpu: Make the fpu state change in fpu__clear() scheduler-atomic
x86/fpu: Split the state handling in fpu__drop()
x86/fpu: Change fpu->fpregs_active users to fpu->fpstate_active
x86/fpu: Decouple fpregs_activate()/fpregs_deactivate() from fpu->fpregs_active x86/fpu: Remove struct fpu::fpregs_active
x86/fpu: Fix fpu__activate_fpstate_read() and update comments
x86/fpu: Remove fpu__current_fpstate_write_begin/end()
x86/fpu: Rename fpu::fpstate_active to fpu::initialized
x86/fpu: Fix stale comments about lazy FPU logic
x86/fpu: Simplify and speed up fpu__copy()
x86/fpu: Rename fpu__activate_curr() to fpu__initialize()
x86/fpu: Rename fpu__activate_fpstate_read/write() to fpu__prepare_[read|write]()
Rik van Riel (1):
x86/fpu: Add FPU state copying quirk to handle XRSTOR failure on Intel Skylake CPUs
kbuild test robot (1):
x86/fpu: Fix boolreturn.cocci warnings
arch/x86/ia32/ia32_signal.c | 2 +- arch/x86/include/asm/fpu/internal.h | 90 +++--------- arch/x86/include/asm/fpu/types.h | 32 +---- arch/x86/include/asm/fpu/xstate.h | 12 +- arch/x86/include/asm/trace/fpu.h | 11 +- arch/x86/kernel/fpu/core.c | 155 ++++++--------------- arch/x86/kernel/fpu/init.c | 2 +- arch/x86/kernel/fpu/regset.c | 48 ++++--- arch/x86/kernel/fpu/signal.c | 37 ++--- arch/x86/kernel/fpu/xstate.c | 264 +++++++++++++++++++++++++++++------- arch/x86/kernel/signal.c | 6 +- arch/x86/kvm/x86.c | 2 +- arch/x86/math-emu/fpu_entry.c | 2 +- arch/x86/mm/extable.c | 24 ++++ arch/x86/mm/pkeys.c | 3 +- 15 files changed, 375 insertions(+), 315 deletions(-)
arch/x86/ia32/ia32_signal.c +1 -1
@@ -231,7 +231,7 @@ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs, ksig->ka.sa.sa_restorer) sp = (unsigned long ) ksig->ka.sa.sa_restorer; -if (fpu->fpstate_active) { +if (fpu->initialized) {unsigned long fx_aligned, math_size; sp =fpu__alloc_mathframe (sp,1 , &fx_aligned, &math_size);
arch/x86/include/asm/fpu/internal.h +22 -68
193 lines not on this page · raw message
arch/x86/include/asm/fpu/types.h +6 -26
@@ -68,6 +68,9 @@ struct fxregs_state {/* Default value for fxregs_state.mxcsr: */ #define MXCSR_DEFAULT 0x1f80 +/* Copy both mxcsr & mxcsr_flags with a single u64 memcpy: */ +#define MXCSR_AND_FLAGS_SIZE sizeof(u64) +/* * Software based FPU emulation state. This is arbitrary really, * it matches the x87 format to make it easier to understand:
@@ -290,36 +293,13 @@ struct fpu {unsigned int last_cpu;/* -* @fpstate_active: +* @initialized: * -* This flag indicates whether this context is active: if the task +* This flag indicates whether this context is initialized: if the task * is not running then we can restore from this context, if the task * is running then we should save into this context. */ -unsigned char fpstate_active; - -/* -* @fpregs_active: -* -* This flag determines whether a given context is actively -* loaded into the FPU's registers and that those registers -* represent the task's current FPU state. -* -* Note the interaction with fpstate_active: -* -* # task does not use the FPU: -* fpstate_active == 0 -* -* # task uses the FPU and regs are active: -* fpstate_active == 1 && fpregs_active == 1 -* -* # the regs are inactive but still match fpstate: -* fpstate_active == 1 && fpregs_active == 0 && fpregs_owner == fpu -* -* The third state is what we use for the lazy restore optimization -* on lazy-switching CPUs. -*/ -unsigned char fpregs_active; +unsigned char initialized;/* * @state:
arch/x86/include/asm/fpu/xstate.h +8 -4
@@ -48,8 +48,12 @@ 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 copyout_from_xsaves (unsigned int pos,unsigned int count,void *kbuf, -void __user *ubuf,struct xregs_state *xsave); -int copyin_to_xsaves (const void *kbuf,const void __user *ubuf, -struct xregs_state *xsave); +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 (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
arch/x86/include/asm/trace/fpu.h +4 -7
@@ -12,25 +12,22 @@ DECLARE_EVENT_CLASS(x86_fpu,TP_STRUCT__entry (__field (struct fpu *,fpu ) -__field (bool, fpregs_active) -__field (bool, fpstate_active) +__field (bool, initialized)__field (u64, xfeatures)__field (u64, xcomp_bv) ),TP_fast_assign ( __entry->fpu = fpu; - __entry->fpregs_active = fpu->fpregs_active; - __entry->fpstate_active = fpu->fpstate_active; + __entry->initialized = fpu->initialized;if (boot_cpu_has (X86_FEATURE_OSXSAVE )) { __entry->xfeatures = fpu->state.xsave.header.xfeatures; __entry->xcomp_bv = fpu->state.xsave.header.xcomp_bv; } ), -TP_printk ("x86/fpu: %p fpregs_active: %d fpstate_active: %d xfeatures: %llx xcomp_bv: %llx" , +TP_printk ("x86/fpu: %p initialized: %d xfeatures: %llx xcomp_bv: %llx" , __entry->fpu, - __entry->fpregs_active, - __entry->fpstate_active, + __entry->initialized, __entry->xfeatures, __entry->xcomp_bv )
arch/x86/kernel/fpu/core.c +43 -112
275 lines not on this page · raw message
arch/x86/kernel/fpu/init.c +1 -1
@@ -240,7 +240,7 @@ static void __init fpu__init_system_ctx_switch(void)WARN_ON_FPU (!on_boot_cpu); on_boot_cpu =0 ; -WARN_ON_FPU (current->thread.fpu.fpstate_active); +WARN_ON_FPU (current->thread.fpu.initialized); }/*
arch/x86/kernel/fpu/regset.c +26 -22
113 lines not on this page · raw message
arch/x86/kernel/fpu/signal.c +21 -16
@@ -155,7 +155,8 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)*/ int copy_fpstate_to_sigframe (void __user *buf,void __user *buf_fx,int size) { -struct xregs_state *xsave = ¤t->thread.fpu.state.xsave; +struct fpu *fpu = ¤t->thread.fpu; +struct xregs_state *xsave = &fpu->state.xsave;struct task_struct *tsk = current;int ia32_fxstate = (buf != buf_fx);
@@ -170,13 +171,13 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)sizeof (struct user_i387_ia32_struct ),NULL , (struct _fpstate_32 __user *) buf) ? -1 : 1; -if (fpregs_active () ||using_compacted_format ()) { +if (fpu->initialized ||using_compacted_format ()) {/* Save the live register state to the user directly. */ if (copy_fpregs_to_sigframe (buf_fx))return -1 ;/* Update the thread's fxstate to save the fsave header. */ if (ia32_fxstate) -copy_fxregs_to_kernel (&tsk->thread.fpu); +copy_fxregs_to_kernel (fpu); }else {/* * It is a *bug* if kernel uses compacted-format for xsave
@@ -189,7 +190,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)return -1 ; } -fpstate_sanitize_xstate (&tsk->thread.fpu); +fpstate_sanitize_xstate (fpu);if (__copy_to_user (buf_fx, xsave, fpu_user_xstate_size))return -1 ; }
@@ -213,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
@@ -223,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 ()) {
@@ -279,7 +283,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)if (!access_ok (VERIFY_READ , buf, size))return -EACCES ; -fpu__activate_curr (fpu); +fpu__initialize (fpu);if (!static_cpu_has (X86_FEATURE_FPU )) return fpregs_soft_set(current, NULL,
@@ -307,28 +311,29 @@ 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;int err =0 ;/* -* Drop the current fpu which clears fpu->fpstate_active. This ensures +* Drop the current fpu which clears fpu->initialized. This ensures * that any context-switch during the copy of the new state, * avoids the intermediate state from getting restored/saved. * Thus avoiding the new restored state from getting corrupted. * We will be ready to restore/save the state only after -* fpu->fpstate_active is again set. +* fpu->initialized is again set. */ fpu__drop (fpu);if (using_compacted_format ()) { - err =copyin_to_xsaves (NULL , 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); + err =__copy_from_user (&fpu->state.xsave, buf_fx, state_size); + +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))) {
@@ -339,7 +344,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)sanitize_restored_xstate (tsk, &env, xfeatures, fx_only); } - fpu->fpstate_active =1 ; + fpu->initialized =1 ;preempt_disable ();fpu__restore (fpu);preempt_enable ();
arch/x86/kernel/fpu/xstate.c +213 -51
366 lines not on this page · raw message
arch/x86/kernel/signal.c +3 -3
@@ -263,7 +263,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size, sp = (unsigned long ) ka->sa.sa_restorer; } -if (fpu->fpstate_active) { +if (fpu->initialized) { sp =fpu__alloc_mathframe (sp,IS_ENABLED (CONFIG_X86_32 ), &buf_fx, &math_size); *fpstate = (void __user *)sp;
@@ -279,7 +279,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,return (void __user *)-1L ;/* save i387 and extended state */ -if (fpu->fpstate_active && +if (fpu->initialized &©_fpstate_to_sigframe (*fpstate, (void __user *)buf_fx, math_size) <0 )return (void __user *)-1L ;
@@ -755,7 +755,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)/* * Ensure the signal handler starts with the new fpu state. */ -if (fpu->fpstate_active) +if (fpu->initialized)fpu__clear (fpu); }signal_setup_done (failed, ksig, stepping);
arch/x86/kvm/x86.c +1 -1
@@ -7225,7 +7225,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)int r;sigset_t sigsaved; -fpu__activate_curr (fpu); +fpu__initialize (fpu);if (vcpu->sigset_active)sigprocmask (SIG_SETMASK , &vcpu->sigset, &sigsaved);
arch/x86/math-emu/fpu_entry.c +1 -1
@@ -114,7 +114,7 @@ void math_emulate(struct math_emu_info *info)struct desc_struct code_descriptor;struct fpu *fpu = ¤t->thread.fpu; -fpu__activate_curr (fpu); +fpu__initialize (fpu);#ifdef RE_ENTRANT_CHECKING if (emulating) {
arch/x86/mm/extable.c +24 -0
@@ -2,6 +2,7 @@#include <linux/uaccess.h> #include <linux/sched/debug.h> +#include <asm/fpu/internal.h> #include <asm/traps.h> #include <asm/kdebug.h>
@@ -78,6 +79,29 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup, }EXPORT_SYMBOL_GPL (ex_handler_refcount); +/* +* Handler for when we fail to restore a task's FPU state. We should never get +* here because the FPU state of a task using the FPU (task->thread.fpu.state) +* should always be valid. However, past bugs have allowed userspace to set +* reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn(). +* These caused XRSTOR to fail when switching to the task, leaking the FPU +* registers of the task previously executing on the CPU. Mitigate this class +* of vulnerability by restoring from the initial state (essentially, zeroing +* out all the FPU registers) if we can't restore from the task's FPU state. +*/ +bool ex_handler_fprestore (const struct exception_table_entry *fixup, +struct pt_regs *regs,int trapnr) +{ + regs->ip =ex_fixup_addr (fixup); + +WARN_ONCE (1 ,"Bad FPU state detected at %pB, reinitializing FPU registers." , + (void *)instruction_pointer (regs)); + +__copy_kernel_to_fpregs (&init_fpstate,-1 ); +return true; +} +EXPORT_SYMBOL_GPL (ex_handler_fprestore); +bool ex_handler_ext (const struct exception_table_entry *fixup,struct pt_regs *regs,int trapnr) {
arch/x86/mm/pkeys.c +1 -2
@@ -18,7 +18,6 @@#include <asm/cpufeature.h> /* boot_cpu_has, ... */ #include <asm/mmu_context.h> /* vma_pkey() */ -#include <asm/fpu/internal.h> /* fpregs_active() */ int __execute_only_pkey (struct mm_struct *mm) {
@@ -45,7 +44,7 @@ int __execute_only_pkey(struct mm_struct *mm)*/ preempt_disable ();if (!need_to_set_mm_pkey && -fpregs_active () && + current->thread.fpu.initialized && !__pkru_allows_read (read_pkru (), execute_only_pkey)) {preempt_enable ();return execute_only_pkey;