Messages in this thread | | | Date | Fri, 2 Jul 2021 15:21:22 -0700 | From | Andrei Vagin <> | Subject | Re: [PATCH 2/4] arch/x86: implement the process_vm_exec syscall |
| |
On Fri, Jul 02, 2021 at 10:51:13AM +0200, Peter Zijlstra wrote: > > I'm terrified of all of this... > > On Tue, Apr 13, 2021 at 10:52:15PM -0700, Andrei Vagin wrote: > > > +long swap_vm_exec_context(struct sigcontext __user *uctx) > > +{ > > + struct sigcontext ctx = {}; > > + sigset_t set = {}; > > + > > + > > + if (copy_from_user(&ctx, uctx, CONTEXT_COPY_SIZE)) > > + return -EFAULT; > > + /* A floating point state is managed from user-space. */ > > + if (ctx.fpstate != 0) > > + return -EINVAL;
Here, we check that ctx doesn't have an FPU state.
> > + if (!user_access_begin(uctx, sizeof(*uctx))) > > + return -EFAULT; > > + unsafe_put_sigcontext(uctx, NULL, current_pt_regs(), (&set), Efault); > > + user_access_end(); > > But here you save the sigcontext without FPU state. > > > + > > + if (__restore_sigcontext(current_pt_regs(), &ctx, 0)) > > + goto badframe; > > And here you restore sigcontext, *with* FPU state. At which point your > FPU state is irrecoverably lost.
process_vm_exec doesn't change a process FPU state. Unlike signals, here we can control it from a user-space. A process can set an FPU state before process_vm_exec and then retore its FPU state after the call.
This version of patches has a bug that I fixed in my tree when I implemented the user-space part for gVisor. I didn't take into account that restore_sigcontext(ctx) clears a process fpu state if ctx->fpstate is zero. I moved fpu__restore_sig out from __restore_sigcontext to fix this issue:
https://github.com/avagin/linux-task-diag/commit/55b7194d00ff
> > Also, I'm not at all convinced this can ever do the right thing when the > tasks don't agree on what the FPU state is. I suppose in the best case > the save will EFAULT. > > > + > > + return 0; > > +Efault: > > + user_access_end(); > > +badframe: > > + signal_fault(current_pt_regs(), uctx, "swap_vm_exec_context"); > > + return -EFAULT; > > +}
| |