[RFC PATCH] um: mark rodata read-only and implement _nofault accesses

Berg, Benjamin benjamin.berg at intel.com
Sun Nov 3 06:46:28 PST 2024 

Hi,

there is a small bug that causes issues with this patch, see below.

On Thu, 2024-10-10 at 23:12 +0200, Johannes Berg wrote:
> [SNIP]
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index cdaee3e94273..378e6e1815b9 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -16,6 +16,7 @@
>  #include <kern_util.h>
>  #include <os.h>
>  #include <skas.h>
> +#include <arch.h>
>  
>  /*
>   * Note this is constrained to return 0, -EFAULT, -EACCES, -ENOMEM by
> @@ -180,7 +181,8 @@ void fatal_sigsegv(void)
>   * If the userfault did not happen in an UML userspace process, bad_segv is called.
>   * Otherwise the signal did happen in a cloned userspace process, handle it.
>   */
> -void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
> +void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs,
> +		  void *mc)
>  {
>  	struct faultinfo * fi = UPT_FAULTINFO(regs);
>  
> @@ -189,7 +191,7 @@ void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
>  		bad_segv(*fi, UPT_IP(regs));
>  		return;
>  	}
> -	segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> +	segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs, mc);
>  }
>  
>  /*
> @@ -199,7 +201,7 @@ void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
>   * give us bad data!
>   */
>  unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> -		   struct uml_pt_regs *regs)
> +		   struct uml_pt_regs *regs, void *mc)
>  {
>  	int si_code;
>  	int err;
> @@ -223,6 +225,19 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>  		goto out;
>  	}
>  	else if (current->mm == NULL) {
> +		if (current->pagefault_disabled) {
> +			if (!mc) {
> +				show_regs(container_of(regs, struct pt_regs, regs));
> +				panic("Segfault with pagefaults disabled but no mcontext");
> +			}
> +			if (!current->thread.segv_continue) {
> +				show_regs(container_of(regs, struct pt_regs, regs));
> +				panic("Segfault without recovery target");
> +			}
> +			mc_set_rip(mc, current->thread.segv_continue);
> +			current->thread.segv_continue = NULL;
> +			return 0;

This needs to be "goto out". Otherwise segv_regs is still set, which
will result in a crash later on if the stacktrace code reads an
incorrect stack pointer from there.

Benjamin

> +		}
>  		show_regs(container_of(regs, struct pt_regs, regs));
>  		panic("Segfault with no mm");
>  	}
> @@ -274,7 +289,8 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>  	return 0;
>  }
>  
> -void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs *regs)
> +void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs *regs,
> +		  void *mc)
>  {
>  	int code, err;
>  	if (!UPT_IS_USER(regs)) {
> @@ -302,7 +318,8 @@ void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs *regs)
>  	}
>  }
>  
> -void winch(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
> +void winch(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs,
> +	   void *mc)
>  {
>  	do_IRQ(WINCH_IRQ, regs);
>  }
> diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c
> index 1978eaa557e9..2aa02657ee78 100644
> --- a/arch/um/os-Linux/signal.c
> +++ b/arch/um/os-Linux/signal.c
> @@ -21,7 +21,7 @@
>  #include <sys/ucontext.h>
>  #include <timetravel.h>
>  
> -void (*sig_info[NSIG])(int, struct siginfo *, struct uml_pt_regs *) = {
> +void (*sig_info[NSIG])(int, struct siginfo *, struct uml_pt_regs *, void *mc) = {
>  	[SIGTRAP]	= relay_signal,
>  	[SIGFPE]	= relay_signal,
>  	[SIGILL]	= relay_signal,
> @@ -47,7 +47,7 @@ static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
>  	if ((sig != SIGIO) && (sig != SIGWINCH))
>  		unblock_signals_trace();
>  
> -	(*sig_info[sig])(sig, si, &r);
> +	(*sig_info[sig])(sig, si, &r, mc);
>  
>  	errno = save_errno;
>  }
> diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
> index 2286486bb0f3..1447be7a4bf2 100644
> --- a/arch/um/os-Linux/skas/process.c
> +++ b/arch/um/os-Linux/skas/process.c
> @@ -166,7 +166,7 @@ static void get_skas_faultinfo(int pid, struct faultinfo *fi)
>  static void handle_segv(int pid, struct uml_pt_regs *regs)
>  {
>  	get_skas_faultinfo(pid, &regs->faultinfo);
> -	segv(regs->faultinfo, 0, 1, NULL);
> +	segv(regs->faultinfo, 0, 1, NULL, NULL);
>  }
>  
>  static void handle_trap(int pid, struct uml_pt_regs *regs)
> @@ -489,7 +489,7 @@ void userspace(struct uml_pt_regs *regs)
>  					get_skas_faultinfo(pid,
>  							   &regs->faultinfo);
>  					(*sig_info[SIGSEGV])(SIGSEGV, (struct siginfo *)&si,
> -							     regs);
> +							     regs, NULL);
>  				}
>  				else handle_segv(pid, regs);
>  				break;
> @@ -497,7 +497,7 @@ void userspace(struct uml_pt_regs *regs)
>  				handle_trap(pid, regs);
>  				break;
>  			case SIGTRAP:
> -				relay_signal(SIGTRAP, (struct siginfo *)&si, regs);
> +				relay_signal(SIGTRAP, (struct siginfo *)&si, regs, NULL);
>  				break;
>  			case SIGALRM:
>  				break;
> @@ -507,7 +507,7 @@ void userspace(struct uml_pt_regs *regs)
>  			case SIGFPE:
>  			case SIGWINCH:
>  				block_signals_trace();
> -				(*sig_info[sig])(sig, (struct siginfo *)&si, regs);
> +				(*sig_info[sig])(sig, (struct siginfo *)&si, regs, NULL);
>  				unblock_signals_trace();
>  				break;
>  			default:
> diff --git a/arch/x86/um/os-Linux/mcontext.c b/arch/x86/um/os-Linux/mcontext.c
> index e80ab7d28117..d2f3a595b4ef 100644
> --- a/arch/x86/um/os-Linux/mcontext.c
> +++ b/arch/x86/um/os-Linux/mcontext.c
> @@ -4,6 +4,7 @@
>  #include <asm/ptrace.h>
>  #include <sysdep/ptrace.h>
>  #include <sysdep/mcontext.h>
> +#include <arch.h>
>  
>  void get_regs_from_mc(struct uml_pt_regs *regs, mcontext_t *mc)
>  {
> @@ -31,3 +32,14 @@ void get_regs_from_mc(struct uml_pt_regs *regs, mcontext_t *mc)
>  	regs->gp[CS / sizeof(unsigned long)] |= 3;
>  #endif
>  }
> +
> +void mc_set_rip(void *_mc, void *target)
> +{
> +	mcontext_t *mc = _mc;
> +
> +#ifdef __i386__
> +	mc->gregs[REG_EIP] = (unsigned long)target;
> +#else
> +	mc->gregs[REG_RIP] = (unsigned long)target;
> +#endif
> +}
> diff --git a/arch/x86/um/shared/sysdep/faultinfo_32.h b/arch/x86/um/shared/sysdep/faultinfo_32.h
> index b6f2437ec29c..ab5c8e47049c 100644
> --- a/arch/x86/um/shared/sysdep/faultinfo_32.h
> +++ b/arch/x86/um/shared/sysdep/faultinfo_32.h
> @@ -29,4 +29,16 @@ struct faultinfo {
>  
>  #define PTRACE_FULL_FAULTINFO 0
>  
> +#define ___backtrack_faulted(_faulted)					\
> +	asm volatile (							\
> +		"mov $0, %0\n"						\
> +		"movl $__get_kernel_nofault_faulted_%=,%1\n"		\
> +		"jmp _end_%=\n"						\
> +		"__get_kernel_nofault_faulted_%=:\n"			\
> +		"mov $1, %0;"						\
> +		"_end_%=:"						\
> +		: "=r" (_faulted),					\
> +		  "=m" (current->thread.segv_continue) ::		\
> +	)
> +
>  #endif
> diff --git a/arch/x86/um/shared/sysdep/faultinfo_64.h b/arch/x86/um/shared/sysdep/faultinfo_64.h
> index ee88f88974ea..26fb4835d3e9 100644
> --- a/arch/x86/um/shared/sysdep/faultinfo_64.h
> +++ b/arch/x86/um/shared/sysdep/faultinfo_64.h
> @@ -29,4 +29,16 @@ struct faultinfo {
>  
>  #define PTRACE_FULL_FAULTINFO 1
>  
> +#define ___backtrack_faulted(_faulted)					\
> +	asm volatile (							\
> +		"mov $0, %0\n"						\
> +		"movq $__get_kernel_nofault_faulted_%=,%1\n"		\
> +		"jmp _end_%=\n"						\
> +		"__get_kernel_nofault_faulted_%=:\n"			\
> +		"mov $1, %0;"						\
> +		"_end_%=:"						\
> +		: "=r" (_faulted),					\
> +		  "=m" (current->thread.segv_continue) ::		\
> +	)
> +
>  #endif

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

More information about the linux-um mailing list