Messages in this thread | | | Date | Wed, 16 Jun 2021 17:22:23 +0200 | From | Borislav Petkov <> | Subject | Re: [patch V2 11/52] x86/fpu: Rewrite xfpregs_set() |
| |
On Mon, Jun 14, 2021 at 05:44:19PM +0200, Thomas Gleixner wrote: > From: Andy Lutomirski <luto@kernel.org> > > xfpregs_set() was incomprehensible. Almost all of the complexity was due > to trying to support nonsensically sized writes or -EFAULT errors that > would have partially or completely overwritten the destination before > failing. Nonsensically sized input would only have been possible using > PTRACE_SETREGSET on REGSET_XFP. Fortunately, it appears (based on Debian > code search results) that no one uses that API at all, let alone with the > wrong sized buffer. Failed user access can be handled more cleanly by > first copying to kernel memory. > > Just rewrite it to require sensible input. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: New patch picked up from Andy > --- > arch/x86/kernel/fpu/regset.c | 40 +++++++++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > --- a/arch/x86/kernel/fpu/regset.c > +++ b/arch/x86/kernel/fpu/regset.c > @@ -47,30 +47,40 @@ int xfpregs_set(struct task_struct *targ > const void *kbuf, const void __user *ubuf) > { > struct fpu *fpu = &target->thread.fpu; > + struct user32_fxsr_struct newstate; > int ret; > > - if (!boot_cpu_has(X86_FEATURE_FXSR)) > + BUILD_BUG_ON(sizeof(newstate) != sizeof(struct fxregs_state)); > + > + if (!static_cpu_has(X86_FEATURE_FXSR))
cpu_feature_enabled
> return -ENODEV; > > - fpu__prepare_write(fpu); > - fpstate_sanitize_xstate(fpu); > + /* No funny business with partial or oversized writes is permitted. */ > + if (pos != 0 || count != sizeof(newstate)) > + return -EINVAL; > > ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > - &fpu->state.fxsave, 0, -1); > + &newstate, 0, -1);
Let that line stick out.
> + if (ret) > + return ret; > + > + /* Mask invalid MXCSR bits (for historical reasons). */
security reasons became historical reasons huh? :-)
With those fixed:
Reviewed-by: Borislav Petkov <bp@suse.de>
Thx.
-- Regards/Gruss, Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
| |