lkml.org 
[lkml]   [2021]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2021-06-07 21:40    [W:1.226 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site