lkml.org 
[lkml]   [2021]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [patch 0/8] x86/fpu: Mop up XSAVES and related damage
Date
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)
{
\
 
 \ /
  Last update: 2021-06-05 13:57    [W:0.062 / U:1.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site