Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | Re: [patch 0/8] x86/fpu: Mop up XSAVES and related damage | Date | Sat, 05 Jun 2021 13:56:19 +0200 |
| |
On Sat, Jun 05 2021 at 12:18, Thomas Gleixner wrote: > On Fri, Jun 04 2021 at 15:04, Dave Hansen wrote: >> No bug is jumping out of the code as I took a brief look at it. The >> xbuf versus kbuf code looks a bit wonky, but I can't find a hole in it. > > I can.... > > --- a/arch/x86/kernel/fpu/regset.c > +++ b/arch/x86/kernel/fpu/regset.c > @@ -128,7 +128,7 @@ int xstateregs_set(struct task_struct *t > xbuf = vmalloc(count); > if (!xbuf) > return -ENOMEM; > - ret = user_regset_copyin(&pos, &count, NULL, &ubuf, xbuf, 0, -1); > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xbuf, 0, -1); > if (ret) > goto out; > }
But this whole user_regset_copyin() is pointless here. See below.
Thanks,
tglx --- include/asm/fpu/xstate.h | 4 ---- kernel/fpu/regset.c | 40 ++++++++++++++++------------------------ kernel/fpu/xstate.c | 12 +++++++----- 3 files changed, 23 insertions(+), 33 deletions(-)
--- a/arch/x86/include/asm/fpu/xstate.h +++ b/arch/x86/include/asm/fpu/xstate.h @@ -112,8 +112,4 @@ void copy_supervisor_to_kernel(struct xr void copy_dynamic_supervisor_to_kernel(struct xregs_state *xstate, u64 mask); void copy_kernel_to_dynamic_supervisor(struct xregs_state *xstate, u64 mask); - -/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */ -int validate_user_xstate_header(const struct xstate_header *hdr); - #endif --- a/arch/x86/kernel/fpu/regset.c +++ b/arch/x86/kernel/fpu/regset.c @@ -6,6 +6,9 @@ #include <asm/fpu/signal.h> #include <asm/fpu/regset.h> #include <asm/fpu/xstate.h> + +#include <linux/vmalloc.h> + #include <linux/sched/task_stack.h> /* @@ -108,7 +111,7 @@ int xstateregs_set(struct task_struct *t const void *kbuf, const void __user *ubuf) { struct fpu *fpu = &target->thread.fpu; - struct xregs_state *xsave; + struct xregs_state *xbuf = NULL; int ret; if (!boot_cpu_has(X86_FEATURE_XSAVE)) @@ -120,32 +123,21 @@ int xstateregs_set(struct task_struct *t if (pos != 0 || count != fpu_user_xstate_size) return -EFAULT; - xsave = &fpu->state.xsave; - - fpu__prepare_write(fpu); - - 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_user_xstate_header(&xsave->header); + if (!kbuf) { + xbuf = vmalloc(count); + if (!xbuf) + return -ENOMEM; + if (copy_from_user(xbuf, ubuf, count)) { + ret = -EFAULT; + goto out; + } } - /* - * mxcsr reserved bits must be masked to zero for security reasons. - */ - xsave->i387.mxcsr &= mxcsr_feature_mask; - - /* - * In case of failure, mark all states as init: - */ - if (ret) - fpstate_init(&fpu->state); + fpu__prepare_write(fpu); + ret = copy_kernel_to_xstate(&fpu->state.xsave, kbuf ? kbuf : xbuf); +out: + vfree(xbuf); return ret; } --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -515,7 +515,7 @@ int using_compacted_format(void) } /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */ -int validate_user_xstate_header(const struct xstate_header *hdr) +static int validate_user_xstate_header(const struct xstate_header *hdr) { /* No unknown or supervisor features may be set */ if (hdr->xfeatures & ~xfeatures_mask_user()) @@ -1172,14 +1172,16 @@ int copy_kernel_to_xstate(struct xregs_s */ xsave->header.xfeatures |= hdr.xfeatures; + /* mxcsr reserved bits must be masked to zero for security reasons. */ + xsave->i387.mxcsr &= mxcsr_feature_mask; + return 0; } /* - * 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. + * Convert from a sigreturn standard-format user-space buffer to kernel + * XSAVES format and copy to the target thread. This is called from the + * sigreturn() and rt_sigreturn() system calls. */ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) {
| |