[haiku-commits] Re: BRANCH pdziepak-github.memcpy-v2 [c4c0758] src/system/kernel/arch/x86/64 src/system/libroot/posix/string/arch/x86_64 src/system/kernel/arch/x86 headers/private/kernel/arch/x86/64

  • From: Alex Smith <alex@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sat, 13 Sep 2014 09:40:37 +0100

On 10 September 2014 21:31, pdziepak-github.memcpy-v2
<community@xxxxxxxxxxxx> wrote:
> added 2 changesets to branch 'refs/remotes/pdziepak-github/memcpy-v2'
> old head: bb159c73fb99410fb591ee25b1fe0b92cd5870a5
> new head: c4c07587ea65bf0a7854ca91204324301b72e8df
> overview: https://github.com/pdziepak/Haiku/compare/bb159c7...c4c0758
>
> ----------------------------------------------------------------------------
>
> f4c3362: kernel/x86_64: save fpu state at interrupts
>
>   The kernel is allowed to use fpu anywhere so we must make sure that
>   user state is not clobbered by saving fpu state at interrupt entry.
>   There is no need to do that in case of system calls since all fpu
>   data registers are caller saved.

If you don't save and restore the FPU state across a syscall, wouldn't
that leave open the possibility of leaking some kernel data back to
userland in the FPU registers, particularly since you're using SSE for
memcpy?

<snip>
> diff --git a/src/system/kernel/arch/x86/64/thread.cpp 
> b/src/system/kernel/arch/x86/64/thread.cpp
> index 961a253..9e535fe 100644
> --- a/src/system/kernel/arch/x86/64/thread.cpp
> +++ b/src/system/kernel/arch/x86/64/thread.cpp
> @@ -134,9 +134,12 @@ arch_thread_init(kernel_args* args)
>  {
>         // Save one global valid FPU state; it will be copied in the arch 
> dependent
>         // part of each new thread.
> -       asm volatile ("clts; fninit; fnclex;");
> -       x86_fxsave(sInitialState.fpu_state);
> -
> +       asm volatile (
> +               "clts;"         \
> +               "fninit;"       \
> +               "fnclex;"       \
> +               "fxsave %0;"
> +               : "=m" (sInitialState.fpu_state));
>         return B_OK;
>  }
>
> @@ -296,15 +299,14 @@ arch_setup_signal_frame(Thread* thread, struct 
> sigaction* action,
>         signalFrameData->context.uc_mcontext.rip = frame->ip;
>         signalFrameData->context.uc_mcontext.rflags = frame->flags;
>
> -       // Store the FPU state. There appears to be a bug in GCC where the 
> aligned
> -       // attribute on a structure is being ignored when the structure is 
> allocated
> -       // on the stack, so even if the fpu_state struct has aligned(16) it 
> may not
> -       // get aligned correctly. Instead, use the current thread's FPU save 
> area
> -       // and then memcpy() to the frame structure.
> -       x86_fxsave(thread->arch_info.fpu_state);
> -       memcpy((void*)&signalFrameData->context.uc_mcontext.fpu,
> -               thread->arch_info.fpu_state,
> -               sizeof(signalFrameData->context.uc_mcontext.fpu));
> +       if (frame->fpu != nullptr) {
> +               memcpy((void*)&signalFrameData->context.uc_mcontext.fpu, 
> frame->fpu,
> +                       sizeof(signalFrameData->context.uc_mcontext.fpu));
> +       } else {
> +               memcpy((void*)&signalFrameData->context.uc_mcontext.fpu,
> +                       sInitialState.fpu_state,
> +                       sizeof(signalFrameData->context.uc_mcontext.fpu));
> +       }

This doesn't look right to me, maybe I'm missing something. I don't
see frame->fpu getting set to !null anywhere except in
arch_restore_signal_frame below and the debug state set function,
which means that in most cases this will just copy the initial state
into the signal frame and not a valid current state.

Thanks,
Alex

Other related posts: