lkml.org 
[lkml]   [2022]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [x86/uaccess] 9c5743dff4: WARNING:at_arch/x86/mm/extable.c:#ex_handler_fprestore
Date
On Fri, May 13 2022 at 09:52, Linus Torvalds wrote:
> On Fri, May 13, 2022 at 1:55 AM kernel test robot <oliver.sang@intel.com> wrote:
> But considering that the fail:runs thing is 41:52, I suspect it's
> something very timing-dependent and who knows how reliable the
> bisection has been.

This smells very much like the issue which got fixed with

59f5ede3bc0f ("x86/fpu: Prevent FPU state corruption")

which resulted in the very same stack trace pattern because the restore
detects the fpstate corruption. The sigframe setup does:

if (TIF_NEED_FPU_LOAD)
restore();
save_to_sigframe();

But yes, in theory it might be caused by ptrace as well. See below.

>> 24: 89 c2 mov %eax,%edx
>> 26: 48 0f ae 2f xrstor64 (%rdi)
>> 2a:* 48 c7 c7 58 47 2b 8c mov $0xffffffff8c2b4758,%rdi <-- trapping instruction
>
> Seems to be just the exception stack chain (ie notice how it's
> pointing to the instruction after the xrstor64, it's not that the
> immediate register move really trapped).

which is caused by ex_handler_fprestore() itself because it stupidly
fixes up regs->ip _before_ the warning. This should obviously be done
afterwards. Without that fixup it would point at xrstor64.

>> 28: 0f 05 syscall
>> 2a:* 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax <-- trapping instruction
>
> and again, it's just pointing back to after the 'syscall' instruction
> that caused this whole chain of events.
>
> Anyway, I *think* that what may be going on is some ptrace thing, but
> let's bring in other people. Because I don't think that "x86/uaccess:
> fix code generation in put_user()" commit is what triggered this, but
> who knows.. The x86 FP code can be very grotty.

Courtesy to the corresponding hardware...

The code which copies the ptrace supplied state has a pile of sanity
checks to catch invalid state, but I wouldn't bet my hat on it that it's
100% complete. We can be more defensive here, but I would be surprised.

Something like the untested below. I'll expose it to some testing to see
what explodes.

Thanks,

tglx
---

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 39e1c8626ab9..c1228d6391c8 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1248,7 +1248,48 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
*/
int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf)
{
- return copy_uabi_to_xstate(fpstate, kbuf, NULL);
+ struct fpstate *tmpfps;
+ unsigned int fpsize;
+ int ret;
+
+ /* This cannot operate on current's fpstate */
+ if (WARN_ON_ONCE(fpstate == current->thread.fpu.fpstate))
+ return -EPERM;
+
+ /* Use a temporary fpstate for the xrstor validation below */
+ fpsize = fpstate->size + ALIGN(offsetof(struct fpstate, regs), 64);
+ tmpfps = vmalloc(fpsize);
+ if (!tmpfps)
+ return -ENOMEM;
+ memcpy(tmpfps, fpstate, fpstate->size);
+
+ ret = copy_uabi_to_xstate(tmpfps, kbuf, NULL);
+ if (ret)
+ goto out;
+ /*
+ * Ensure right here that the user space provided xstate content is
+ * correct. Save current's fpstate and invalidate the per-CPU FPU
+ * state.
+ */
+ kernel_fpu_begin_mask(0);
+ /*
+ * Limit the restore attempt to the user features as fpstate
+ * is not current's fpstate. So current's supervisor state
+ * has to be preserved and the target's supervisor state was
+ * not touched in copy_uabi_to_xstate().
+ */
+ ret = os_xrstor_safe(tmpfps, tmpfps->user_xfeatures);
+ kernel_fpu_end();
+ /*
+ * If the restore succeeded, copy the state. Otherwise
+ * keep the previous content.
+ */
+ if (!ret)
+ memcpy(fpstate, tmpfps, fpstate->size);
+
+out:
+ vfree(tmpfps);
+ return ret;
}

/*
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index dba2197c05c3..c0d852998d18 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -68,11 +68,10 @@ static bool ex_handler_sgx(const struct exception_table_entry *fixup,
static bool ex_handler_fprestore(const struct exception_table_entry *fixup,
struct pt_regs *regs)
{
- regs->ip = ex_fixup_addr(fixup);
-
WARN_ONCE(1, "Bad FPU state detected at %pB, reinitializing FPU registers.",
(void *)instruction_pointer(regs));

+ regs->ip = ex_fixup_addr(fixup);
fpu_reset_from_exception_fixup();
return true;
}




\
 
 \ /
  Last update: 2022-05-15 10:26    [W:0.084 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site