Re: [PATCH] um: always use the internal copy of the FP registers

[Benjamin Berg]

On Fri, 2024-09-13 at 10:22 +0200, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.b...@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.b...@intel.com>

Hi,

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

Benjamin

/*
 * gcc test-signal-restore.c -o test-signal-restore-amd64
 * gcc -m32 -march=i686 -lm test-signal-restore.c -o test-signal-restore-i386
 */

#include <math.h>
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/wait.h>
#include <errno.h>
#include <unistd.h>
#include <sys/ptrace.h>
#include <sys/user.h>
#include <asm/unistd.h>

#define ST0_EXP_ADD 10

void *scratch_page;

void sighandler(int sig, siginfo_t *info, void *p)
{
	ucontext_t *uc = p;

	printf("sighandler\n");

	uc->uc_mcontext.fpregs->_st[0].exponent += ST0_EXP_ADD;
}

int test_fp()
{
	double num = 0.5;
	long ret;

	printf("pre-signal: %g\n", num);
	/*
	 * This does kill(getpid(), SIGUSR1); with "num" being passed in AND
	 * out of the floating point stack. We can therefore modify num by
	 * changing st[0] when handling the signal.
	 */
#ifdef __i386__
	asm volatile (
		"int $0x80;"
		: "=t" (num), "=a" (ret)
		: "0" (num), "1" (__NR_kill), "b" (getpid()), "c" (SIGUSR1) : );
#else
	asm volatile (
		"syscall;"
		: "=t" (num), "=a" (ret)
		: "0" (num), "1" (__NR_kill), "D" (getpid()), "S" (SIGUSR1) : "r11", "rcx");
#endif
	printf("post-signal: %g\n", num);

	if (num != pow(2, ST0_EXP_ADD - 1)) {
		printf("floating point register was not manipulated\n");
		return 1;
	}

	return 0;
}

int test_fp_ptrace(int fpx)
{
	int pid, status, ret;

	pid = fork();
	if (pid < 0)
		return 127;

	if (pid == 0) {
		/* child */
		ptrace(PTRACE_TRACEME, 0, 0, 0);
		kill(getpid(), SIGSTOP);
		
		if (test_fp())
			exit(1);

		exit(0);
	}

	/* Wait for child to stop itself */
	do {
		ret = waitpid(pid, &status, 0);
	} while (ret < 0 && errno == EINTR);
	if (!WIFSTOPPED(status))
		return 127;

	/* Continue until SIGUSR1 to self */
	ptrace(PTRACE_CONT, pid, NULL, 0);
	do {
		ret = waitpid(pid, &status, 0);
	} while (ret < 0 && errno == EINTR);
	if (!WIFSTOPPED(status))
		return 127;

	if (fpx) {
#ifdef __i386__
		struct user_fpxregs_struct *fpstate;
		#define fp_reg_type struct _libc_fpreg
		fpstate = scratch_page + 4096 - sizeof(*fpstate);

		if (ptrace(PTRACE_GETFPXREGS, pid, NULL, fpstate)) {
			kill(pid, SIGKILL);
			if (errno == EINVAL) {
				printf("Getting FPX regs not supported\n");
				return 0;
			} else {
				printf("Error getting FPX regs: %d\n", errno);
				return 127;
			}
		}
		((fp_reg_type*)&fpstate->st_space[0])->exponent += ST0_EXP_ADD;
		if (ptrace(PTRACE_SETFPXREGS, pid, NULL, fpstate))
			return 127;
#else
		printf("No FPXREGS on x86_64\n");
		kill(pid, SIGKILL);
		return 127;
#endif
	} else {
		struct _libc_fpstate *fpstate;
		fpstate = scratch_page + 4096 - sizeof(*fpstate);

		if (ptrace(PTRACE_GETFPREGS, pid, NULL, fpstate)) {
			kill(pid, SIGKILL);
			if (errno == EINVAL) {
				printf("Getting FPX regs not supported\n");
				return 0;
			} else {
				printf("Error getting FPX regs: %d\n", errno);
				return 127;
			}
		}
		fpstate->_st[0].exponent += ST0_EXP_ADD;
		if (ptrace(PTRACE_SETFPREGS, pid, NULL, fpstate))
			return 127;
	}

	/* Run until completion (without handling the signal) */
	ptrace(PTRACE_CONT, pid, NULL, 0);
	do {
		ret = waitpid(pid, &status, 0);
	} while (ret < 0 && errno == EINTR);

	if (!WIFEXITED(status))
		return 127;

	return WEXITSTATUS(status);
}

int main()
{
	struct sigaction sa = {
		.sa_flags = SA_SIGINFO,
		.sa_handler = (void (*)(int))sighandler,
	};
	int ret;

	scratch_page = mmap(NULL, 8192, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	munmap(scratch_page + 4096, 4096);

	sigaction(SIGUSR1, &sa, NULL);

	if (test_fp())
		return 1;

	sa.sa_handler = SIG_DFL;
	sigaction(SIGUSR1, &sa, NULL);

	printf("\nmodify using ptrace PTRACE_GETFPREGS instead of sighandler:\n");
	ret = test_fp_ptrace(0);
	if (ret)
		return ret;

#ifdef __i386__
	printf("\nmodify using ptrace PTRACE_GETFPXREGS instead of sighandler:\n");
	ret = test_fp_ptrace(1);
	if (ret)
		return ret;
#endif

	return 0;
}