diff mbox series

um: always use the internal copy of the FP registers

Message ID 20240913082244.767799-1-benjamin@sipsolutions.net
State Superseded
Headers show
Series um: always use the internal copy of the FP registers | expand

Commit Message

Benjamin Berg Sept. 13, 2024, 8:22 a.m. UTC 
From: Benjamin Berg <benjamin.berg@intel.com>

When switching from userspace to the kernel, all registers including the
FP registers are copied into the kernel and restored later on. As such,
the true source for the FP register state is actually already in the
kernel and they should never be grabbed from the userspace process.

Change the various places to simply copy the data from the internal FP
register storage area. Note that on i386 the format of PTRACE_GETFPREGS
and PTRACE_GETFPXREGS is different enough that conversion would be
needed. With this patch, -EINVAL is returned if the non-native format is
requested.

The upside is, that this patchset fixes setting registers via ptrace
(which simply did not work before) as well as fixing setting floating
point registers using the mcontext on signal return on i386.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>

---

I think that ideally more changs should be done in this space. For a
start, one can remove support for hosts without PTRACE_GETFPXREGS. But,
more importantly, UML should probably implement the regset API, which
would considerably improve both ptrace and coredump generation and
should also make it easy to fix the cases where this patch returns an
error.
---
 arch/um/include/shared/registers.h |  6 ---
 arch/um/kernel/process.c           | 11 ++++-
 arch/x86/um/os-Linux/registers.c   | 12 +++---
 arch/x86/um/ptrace_32.c            | 50 ++++++++++++-----------
 arch/x86/um/ptrace_64.c            | 20 ++++-----
 arch/x86/um/signal.c               | 65 ++++++++++++++----------------
 6 files changed, 80 insertions(+), 84 deletions(-)

Comments

Benjamin Berg Sept. 13, 2024, 8:42 a.m. UTC | #1 
On Fri, 2024-09-13 at 10:22 +0200, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@intel.com>
> 
> When switching from userspace to the kernel, all registers including the
> FP registers are copied into the kernel and restored later on. As such,
> the true source for the FP register state is actually already in the
> kernel and they should never be grabbed from the userspace process.
> 
> Change the various places to simply copy the data from the internal FP
> register storage area. Note that on i386 the format of PTRACE_GETFPREGS
> and PTRACE_GETFPXREGS is different enough that conversion would be
> needed. With this patch, -EINVAL is returned if the non-native format is
> requested.
> 
> The upside is, that this patchset fixes setting registers via ptrace
> (which simply did not work before) as well as fixing setting floating
> point registers using the mcontext on signal return on i386.
> 
> Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>

Hi,

I did test this mcontext restore and ptrace register setting both on
i386 and x86_64 using the attached program.

Benjamin
Tiwei Bie Sept. 13, 2024, 1:09 p.m. UTC | #2 
On 2024/9/13 16:22, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@intel.com>
> 
> When switching from userspace to the kernel, all registers including the
> FP registers are copied into the kernel and restored later on. As such,
> the true source for the FP register state is actually already in the
> kernel and they should never be grabbed from the userspace process.
> 
> Change the various places to simply copy the data from the internal FP
> register storage area.

Cool! I had similar thoughts, but I haven't tried to make it happen.

> Note that on i386 the format of PTRACE_GETFPREGS
> and PTRACE_GETFPXREGS is different enough that conversion would be
> needed. With this patch, -EINVAL is returned if the non-native format is
> requested.
> 
> The upside is, that this patchset fixes setting registers via ptrace
> (which simply did not work before) as well as fixing setting floating
> point registers using the mcontext on signal return on i386.
> 
> Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
> 
> ---
> 
> I think that ideally more changs should be done in this space. For a
> start, one can remove support for hosts without PTRACE_GETFPXREGS. But,
> more importantly, UML should probably implement the regset API, which
> would considerably improve both ptrace and coredump generation and
> should also make it easy to fix the cases where this patch returns an
> error.
> ---
>  arch/um/include/shared/registers.h |  6 ---
>  arch/um/kernel/process.c           | 11 ++++-
>  arch/x86/um/os-Linux/registers.c   | 12 +++---
>  arch/x86/um/ptrace_32.c            | 50 ++++++++++++-----------
>  arch/x86/um/ptrace_64.c            | 20 ++++-----
>  arch/x86/um/signal.c               | 65 ++++++++++++++----------------
>  6 files changed, 80 insertions(+), 84 deletions(-)
> 
> diff --git a/arch/um/include/shared/registers.h b/arch/um/include/shared/registers.h
> index a0450326521c..7d81b2339a48 100644
> --- a/arch/um/include/shared/registers.h
> +++ b/arch/um/include/shared/registers.h
> @@ -8,12 +8,6 @@
>  
>  #include <sysdep/ptrace.h>
>  
> -extern int save_i387_registers(int pid, unsigned long *fp_regs);
> -extern int restore_i387_registers(int pid, unsigned long *fp_regs);
> -extern int save_fp_registers(int pid, unsigned long *fp_regs);
> -extern int restore_fp_registers(int pid, unsigned long *fp_regs);
> -extern int save_fpx_registers(int pid, unsigned long *fp_regs);
> -extern int restore_fpx_registers(int pid, unsigned long *fp_regs);
>  extern int init_pid_registers(int pid);
>  extern void get_safe_registers(unsigned long *regs, unsigned long *fp_regs);
>  extern int get_fp_registers(int pid, unsigned long *regs);
> diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
> index be2856af6d4c..ad798d40f8a4 100644
> --- a/arch/um/kernel/process.c
> +++ b/arch/um/kernel/process.c
> @@ -290,8 +290,15 @@ unsigned long __get_wchan(struct task_struct *p)
>  
>  int elf_core_copy_task_fpregs(struct task_struct *t, elf_fpregset_t *fpu)
>  {
> -	int cpu = current_thread_info()->cpu;
> +#ifdef CONFIG_X86_32
> +	extern int have_fpx_regs;
>  
> -	return save_i387_registers(userspace_pid[cpu], (unsigned long *) fpu);
> +	/* FIXME: A plain copy does not work on i386 with have_fpx_regs */
> +	if (have_fpx_regs)
> +		return -1;
> +#endif
> +	memcpy(fpu, &t->thread.regs.regs.fp, sizeof(*fpu));
> +
> +	return 0;
>  }

This function is expected to return a boolean value which should be
true on success.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_elf.c?h=v6.11-rc7#n1816

Regards,
Tiwei
Benjamin Berg Sept. 13, 2024, 1:17 p.m. UTC | #3 
Hi,

On Fri, 2024-09-13 at 21:09 +0800, Tiwei Bie wrote:
> On 2024/9/13 16:22, Benjamin Berg wrote:
> > From: Benjamin Berg <benjamin.berg@intel.com>
> > [SNIP]
> > diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
> > index be2856af6d4c..ad798d40f8a4 100644
> > --- a/arch/um/kernel/process.c
> > +++ b/arch/um/kernel/process.c
> > @@ -290,8 +290,15 @@ unsigned long __get_wchan(struct task_struct *p)
> >  
> >  int elf_core_copy_task_fpregs(struct task_struct *t, elf_fpregset_t *fpu)
> >  {
> > -	int cpu = current_thread_info()->cpu;
> > +#ifdef CONFIG_X86_32
> > +	extern int have_fpx_regs;
> >  
> > -	return save_i387_registers(userspace_pid[cpu], (unsigned long *) fpu);
> > +	/* FIXME: A plain copy does not work on i386 with have_fpx_regs */
> > +	if (have_fpx_regs)
> > +		return -1;
> > +#endif
> > +	memcpy(fpu, &t->thread.regs.regs.fp, sizeof(*fpu));
> > +
> > +	return 0;
> >  }
> 
> This function is expected to return a boolean value which should be
> true on success.

Hah, of course. And there I was partially doing all of this because of
your patch fixing exactly that problem (and because the current code
cannot work in SECCOMP mode) …

Benjamin
diff mbox series

Patch

diff --git a/arch/um/include/shared/registers.h b/arch/um/include/shared/registers.h
index a0450326521c..7d81b2339a48 100644
--- a/arch/um/include/shared/registers.h
+++ b/arch/um/include/shared/registers.h
@@ -8,12 +8,6 @@ 
 
 #include <sysdep/ptrace.h>
 
-extern int save_i387_registers(int pid, unsigned long *fp_regs);
-extern int restore_i387_registers(int pid, unsigned long *fp_regs);
-extern int save_fp_registers(int pid, unsigned long *fp_regs);
-extern int restore_fp_registers(int pid, unsigned long *fp_regs);
-extern int save_fpx_registers(int pid, unsigned long *fp_regs);
-extern int restore_fpx_registers(int pid, unsigned long *fp_regs);
 extern int init_pid_registers(int pid);
 extern void get_safe_registers(unsigned long *regs, unsigned long *fp_regs);
 extern int get_fp_registers(int pid, unsigned long *regs);
diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index be2856af6d4c..ad798d40f8a4 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -290,8 +290,15 @@  unsigned long __get_wchan(struct task_struct *p)
 
 int elf_core_copy_task_fpregs(struct task_struct *t, elf_fpregset_t *fpu)
 {
-	int cpu = current_thread_info()->cpu;
+#ifdef CONFIG_X86_32
+	extern int have_fpx_regs;
 
-	return save_i387_registers(userspace_pid[cpu], (unsigned long *) fpu);
+	/* FIXME: A plain copy does not work on i386 with have_fpx_regs */
+	if (have_fpx_regs)
+		return -1;
+#endif
+	memcpy(fpu, &t->thread.regs.regs.fp, sizeof(*fpu));
+
+	return 0;
 }
 
diff --git a/arch/x86/um/os-Linux/registers.c b/arch/x86/um/os-Linux/registers.c
index f3638dd09cec..f8e5516d9708 100644
--- a/arch/x86/um/os-Linux/registers.c
+++ b/arch/x86/um/os-Linux/registers.c
@@ -19,14 +19,14 @@ 
 
 static int have_xstate_support;
 
-int save_i387_registers(int pid, unsigned long *fp_regs)
+static int save_i387_registers(int pid, unsigned long *fp_regs)
 {
 	if (ptrace(PTRACE_GETFPREGS, pid, 0, fp_regs) < 0)
 		return -errno;
 	return 0;
 }
 
-int save_fp_registers(int pid, unsigned long *fp_regs)
+static int save_fp_registers(int pid, unsigned long *fp_regs)
 {
 #ifdef PTRACE_GETREGSET
 	struct iovec iov;
@@ -42,14 +42,14 @@  int save_fp_registers(int pid, unsigned long *fp_regs)
 		return save_i387_registers(pid, fp_regs);
 }
 
-int restore_i387_registers(int pid, unsigned long *fp_regs)
+static int restore_i387_registers(int pid, unsigned long *fp_regs)
 {
 	if (ptrace(PTRACE_SETFPREGS, pid, 0, fp_regs) < 0)
 		return -errno;
 	return 0;
 }
 
-int restore_fp_registers(int pid, unsigned long *fp_regs)
+static int restore_fp_registers(int pid, unsigned long *fp_regs)
 {
 #ifdef PTRACE_SETREGSET
 	struct iovec iov;
@@ -66,14 +66,14 @@  int restore_fp_registers(int pid, unsigned long *fp_regs)
 
 #ifdef __i386__
 int have_fpx_regs = 1;
-int save_fpx_registers(int pid, unsigned long *fp_regs)
+static int save_fpx_registers(int pid, unsigned long *fp_regs)
 {
 	if (ptrace(PTRACE_GETFPXREGS, pid, 0, fp_regs) < 0)
 		return -errno;
 	return 0;
 }
 
-int restore_fpx_registers(int pid, unsigned long *fp_regs)
+static int restore_fpx_registers(int pid, unsigned long *fp_regs)
 {
 	if (ptrace(PTRACE_SETFPXREGS, pid, 0, fp_regs) < 0)
 		return -errno;
diff --git a/arch/x86/um/ptrace_32.c b/arch/x86/um/ptrace_32.c
index b0a71c6cdc6e..b8b85a52eb6f 100644
--- a/arch/x86/um/ptrace_32.c
+++ b/arch/x86/um/ptrace_32.c
@@ -168,17 +168,18 @@  int peek_user(struct task_struct *child, long addr, long data)
 	return put_user(tmp, (unsigned long __user *) data);
 }
 
+/* FIXME: Do the required conversions instead of erroring out */
+extern int have_fpx_regs;
+
 static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = task_cpu(child);
-	struct user_i387_struct fpregs;
+	int n;
 
-	err = save_i387_registers(userspace_pid[cpu],
-				  (unsigned long *) &fpregs);
-	if (err)
-		return err;
+	if (have_fpx_regs)
+		return -EINVAL;
 
-	n = copy_to_user(buf, &fpregs, sizeof(fpregs));
+	n = copy_to_user(buf, &child->thread.regs.regs.fp,
+			 sizeof(struct user_i387_struct));
 	if(n > 0)
 		return -EFAULT;
 
@@ -187,27 +188,28 @@  static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 
 static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = task_cpu(child);
-	struct user_i387_struct fpregs;
+	int n;
+
+	if (have_fpx_regs)
+		return -EINVAL;
 
-	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
+	n = copy_from_user(&child->thread.regs.regs.fp, buf,
+			   sizeof(struct user_i387_struct));
 	if (n > 0)
 		return -EFAULT;
 
-	return restore_i387_registers(userspace_pid[cpu],
-				    (unsigned long *) &fpregs);
+	return 0;
 }
 
 static int get_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = task_cpu(child);
-	struct user_fxsr_struct fpregs;
+	int n;
 
-	err = save_fpx_registers(userspace_pid[cpu], (unsigned long *) &fpregs);
-	if (err)
-		return err;
+	if (!have_fpx_regs)
+		return -EINVAL;
 
-	n = copy_to_user(buf, &fpregs, sizeof(fpregs));
+	n = copy_to_user(buf, &child->thread.regs.regs.fp,
+			 sizeof(struct user_fxsr_struct));
 	if(n > 0)
 		return -EFAULT;
 
@@ -216,15 +218,17 @@  static int get_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *
 
 static int set_fpxregs(struct user_fxsr_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = task_cpu(child);
-	struct user_fxsr_struct fpregs;
+	int n;
 
-	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
+	if (!have_fpx_regs)
+		return -EINVAL;
+
+	n = copy_from_user(&child->thread.regs.regs.fp, buf,
+			   sizeof(struct user_fxsr_struct));
 	if (n > 0)
 		return -EFAULT;
 
-	return restore_fpx_registers(userspace_pid[cpu],
-				     (unsigned long *) &fpregs);
+	return 0;
 }
 
 long subarch_ptrace(struct task_struct *child, long request,
diff --git a/arch/x86/um/ptrace_64.c b/arch/x86/um/ptrace_64.c
index aa68d83d3f44..f8bbad29cd0b 100644
--- a/arch/x86/um/ptrace_64.c
+++ b/arch/x86/um/ptrace_64.c
@@ -190,15 +190,10 @@  int peek_user(struct task_struct *child, long addr, long data)
 
 static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
-	struct user_i387_struct fpregs;
+	int n;
 
-	err = save_i387_registers(userspace_pid[cpu],
-				  (unsigned long *) &fpregs);
-	if (err)
-		return err;
-
-	n = copy_to_user(buf, &fpregs, sizeof(fpregs));
+	n = copy_to_user(buf, &child->thread.regs.regs.fp,
+			 sizeof(struct user_i387_struct));
 	if (n > 0)
 		return -EFAULT;
 
@@ -207,15 +202,14 @@  static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 
 static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
-	int n, cpu = ((struct thread_info *) child->stack)->cpu;
-	struct user_i387_struct fpregs;
+	int n;
 
-	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
+	n = copy_from_user(&child->thread.regs.regs.fp, buf,
+			   sizeof(struct user_i387_struct));
 	if (n > 0)
 		return -EFAULT;
 
-	return restore_i387_registers(userspace_pid[cpu],
-				      (unsigned long *) &fpregs);
+	return 0;
 }
 
 long subarch_ptrace(struct task_struct *child, long request,
diff --git a/arch/x86/um/signal.c b/arch/x86/um/signal.c
index 2cc8c2309022..fa78c74e00cb 100644
--- a/arch/x86/um/signal.c
+++ b/arch/x86/um/signal.c
@@ -204,34 +204,30 @@  static int copy_sc_from_user(struct pt_regs *regs,
 
 #ifdef CONFIG_X86_32
 	if (have_fpx_regs) {
-		struct user_fxsr_struct fpx;
-		int pid = userspace_pid[current_thread_info()->cpu];
+		struct user_fxsr_struct *fpx;
+		fpx = (void *)&regs->regs.fp;
 
-		err = copy_from_user(&fpx,
-			&((struct _fpstate __user *)sc.fpstate)->_fxsr_env[0],
-				     sizeof(struct user_fxsr_struct));
+		err = convert_fxsr_from_user(fpx, (void *)sc.fpstate);
 		if (err)
 			return 1;
-
-		err = convert_fxsr_from_user(&fpx, (void *)sc.fpstate);
+	} else {
+		BUILD_BUG_ON(sizeof(regs->regs.fp) > sizeof(struct _fpstate));
+		err = copy_from_user(regs->regs.fp, (void *)sc.fpstate,
+				     sizeof(regs->regs.fp));
 		if (err)
 			return 1;
-
-		err = restore_fpx_registers(pid, (unsigned long *) &fpx);
-		if (err < 0) {
-			printk(KERN_ERR "copy_sc_from_user - "
-			       "restore_fpx_registers failed, errno = %d\n",
-			       -err);
-			return 1;
-		}
-	} else
-#endif
+	}
+#else
 	{
+		/* FIXME: Save/restore extended state (past the 16 YMM regs) */
+		BUILD_BUG_ON(sizeof(regs->regs.fp) < sizeof(struct _xstate));
+
 		err = copy_from_user(regs->regs.fp, (void *)sc.fpstate,
 				     sizeof(struct _xstate));
 		if (err)
 			return 1;
 	}
+#endif
 	return 0;
 }
 
@@ -291,34 +287,35 @@  static int copy_sc_to_user(struct sigcontext __user *to,
 
 #ifdef CONFIG_X86_32
 	if (have_fpx_regs) {
-		int pid = userspace_pid[current_thread_info()->cpu];
-		struct user_fxsr_struct fpx;
+		struct user_fxsr_struct *fpx;
 
-		err = save_fpx_registers(pid, (unsigned long *) &fpx);
-		if (err < 0){
-			printk(KERN_ERR "copy_sc_to_user - save_fpx_registers "
-			       "failed, errno = %d\n", err);
-			return 1;
-		}
+		fpx = (void *)&regs->regs.fp;
 
-		err = convert_fxsr_to_user(&to_fp->fpstate, &fpx);
+		err = convert_fxsr_to_user(&to_fp->fpstate, fpx);
 		if (err)
 			return 1;
 
-		err |= __put_user(fpx.swd, &to_fp->fpstate.status);
+		err |= __put_user(fpx->swd, &to_fp->fpstate.status);
 		err |= __put_user(X86_FXSR_MAGIC, &to_fp->fpstate.magic);
 		if (err)
 			return 1;
 
-		if (copy_to_user(&to_fp->fpstate._fxsr_env[0], &fpx,
-				 sizeof(struct user_fxsr_struct)))
-			return 1;
-	} else
-#endif
-	{
-		if (copy_to_user(to_fp, regs->regs.fp, sizeof(struct _xstate)))
+	} else {
+		if (copy_to_user(to_fp, regs->regs.fp, sizeof(regs->regs.fp)))
 			return 1;
+
+		/* Need to fill in the sw_reserved bits ... */
+		BUILD_BUG_ON(offsetof(typeof(*to_fp),
+				      fpstate.sw_reserved.magic1) >=
+			     sizeof(struct _fpstate));
+		__put_user(0, &to_fp->fpstate.sw_reserved.magic1);
+		__put_user(sizeof(struct _fpstate),
+			   &to_fp->fpstate.sw_reserved.extended_size);
 	}
+#else
+	if (copy_to_user(to_fp, regs->regs.fp, sizeof(struct _xstate)))
+		return 1;
+#endif
 
 	return 0;
 }