Messages in this thread Patch in this message | | | Date | Mon, 7 Jun 2021 21:39:22 +0200 | From | Borislav Petkov <> | Subject | Re: [patch V2 06/14] x86/fpu: Sanitize xstateregs_set() |
| |
On Sun, Jun 06, 2021 at 01:47:48AM +0200, Thomas Gleixner wrote: > xstateregs_set() operates on a stopped task and tries to copy the provided > buffer into the tasks fpu.state.xsave buffer. > > Any error while copying or invalid state detected after copying results in > wiping the target tasks FPU state completely including supervisor states.
Again, "task's" in both above pls.
> That's just wrong. The caller supplied invalid data or has a problem with > unmapped memory, so there is absolutely no justification to wreckage the
Yeah, as amluto already pointed out "wreck the target".
> target. > > Fix this with the following modifications: > > 1) If data has to be copied from userspace, allocate a buffer and copy from > user first. > > 2) Use copy_kernel_to_xstate() unconditionally so that header checking > works correctly. > > 3) Return on error without wreckaging the target state.
"wrecking"
> > This prevents corrupting supervisor states and lets the caller deal with > the problem it caused in the first place. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Move the validate_user_xstate_header() static here - Borislav > Use copy_from_user() - Dave, Yu > Rename xbuf to tmpbuf - Borislav > --- > arch/x86/include/asm/fpu/xstate.h | 4 --- > arch/x86/kernel/fpu/regset.c | 41 +++++++++++++++----------------------- > arch/x86/kernel/fpu/xstate.c | 12 ++++++----- > 3 files changed, 24 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,8 +6,12 @@ > #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>
I think the linux/ namespace headers come first and then the asm/ ones.
IOW, this:
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c index cc49078b74ce..cfa242d54a26 100644 --- a/arch/x86/kernel/fpu/regset.c +++ b/arch/x86/kernel/fpu/regset.c @@ -2,16 +2,15 @@ /* * FPU register's regset abstraction, for ptrace, core dumps, etc. */ + +#include <linux/sched/task_stack.h> +#include <linux/vmalloc.h> + #include <asm/fpu/internal.h> #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> - - /* * The xstateregs_active() routine is the same as the regset_fpregs_active() routine, * as the "regset->n" for the xstate regset will be updated based on the feature
> * The xstateregs_active() routine is the same as the regset_fpregs_active() routine, > * as the "regset->n" for the xstate regset will be updated based on the feature > @@ -108,7 +112,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 *tmpbuf = NULL; > int ret; > > if (!boot_cpu_has(X86_FEATURE_XSAVE))
Can we sneak in a switch to cpu_feature_enabled() too, while at it?
> @@ -120,32 +124,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) { > + tmpbuf = vmalloc(count); > + if (!tmpbuf) > + return -ENOMEM;
<---- newline here to split the vmalloc error handling from the next step. > /* > - * 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
to compacted format.
> + * 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) > { >
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |