* Re: [PATCH v6 1/5] x86/pkeys: Add PKRU as a parameter in signal handling functions
[not found] ` <20240627211737.323214-2-aruna.ramakrishna@oracle.com>
@ 2024-07-17 4:25 ` Jeff Xu
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Xu @ 2024-07-17 4:25 UTC (permalink / raw)
To: Aruna Ramakrishna
Cc: linux-kernel, x86, dave.hansen, tglx, mingo, keith.lucas,
rick.p.edgecombe, Jann Horn, Stephen Röttger,
Jorge Lucangeli Obes, linux-mm, Kees Cook, Jeff Xu
On Thu, Jun 27, 2024 at 2:17 PM Aruna Ramakrishna
<aruna.ramakrishna@oracle.com> wrote:
>
> Problem description:
> Let's assume there's a multithreaded application that runs untrusted
> user code. Each thread has its stack/code protected by a non-zero pkey,
> and the PKRU register is set up such that only that particular non-zero
> pkey is enabled. Each thread also sets up an alternate signal stack to
> handle signals, which is protected by pkey zero. The pkeys man page
> documents that the PKRU will be reset to init_pkru when the signal
> handler is invoked, which means that pkey zero access will be enabled.
> But this reset happens after the kernel attempts to push fpu state
> to the alternate stack, which is not (yet) accessible by the kernel,
> which leads to a new SIGSEGV being sent to the application, terminating
> it.
>
> Enabling both the non-zero pkey (for the thread) and pkey zero in
> userspace will not work for this use case. We cannot have the alt stack
> writeable by all - the rationale here is that the code running in that
> thread (using a non-zero pkey) is untrusted and should not have access
> to the alternate signal stack (that uses pkey zero), to prevent the
> return address of a function from being changed. The expectation is that
> kernel should be able to set up the alternate signal stack and deliver
> the signal to the application even if pkey zero is explicitly disabled
> by the application. The signal handler accessibility should not be
> dictated by whatever PKRU value the thread sets up.
>
> Solution:
> The PKRU register is managed by XSAVE, which means the sigframe contents
> must match the register contents - which is not the case here. We want
> the sigframe to contain the user-defined PKRU value (so that it is
> restored correctly from sigcontext) but the actual register must be
> reset to init_pkru so that the alt stack is accessible and the signal
> can be delivered to the application. It seems that the proper fix here
> would be to remove PKRU from the XSAVE framework and manage it
> separately, which is quite complicated. As a workaround, do this:
>
> orig_pkru = rdpkru();
> wrpkru(orig_pkru & init_pkru_value);
> xsave_to_user_sigframe();
> put_user(pkru_sigframe_addr, orig_pkru)
>
> This change is split over multiple patches.
>
> In preparation for writing PKRU to sigframe in a later patch, pass in
> PKRU as an additional parameter down the chain from handle_signal:
> setup_rt_frame()
> xxx_setup_rt_frame()
Above two functions don't access altstack, therefore we can keep it
the same as before.
> get_sigframe()
> copy_fpstate_to_sigframe()
> copy_fpregs_to_sigframe()
>
> There are no functional changes in this patch.
>
> Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> ---
> arch/x86/include/asm/fpu/signal.h | 2 +-
> arch/x86/include/asm/sighandling.h | 10 +++++-----
> arch/x86/kernel/fpu/signal.c | 6 +++---
> arch/x86/kernel/signal.c | 19 ++++++++++---------
> arch/x86/kernel/signal_32.c | 8 ++++----
> arch/x86/kernel/signal_64.c | 8 ++++----
> 6 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
> index 611fa41711af..eccc75bc9c4f 100644
> --- a/arch/x86/include/asm/fpu/signal.h
> +++ b/arch/x86/include/asm/fpu/signal.h
> @@ -29,7 +29,7 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
>
> unsigned long fpu__get_fpstate_size(void);
>
> -extern bool copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size);
> +extern bool copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size, u32 pkru);
> extern void fpu__clear_user_states(struct fpu *fpu);
> extern bool fpu__restore_sig(void __user *buf, int ia32_frame);
>
> diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
> index e770c4fc47f4..de458354a3ea 100644
> --- a/arch/x86/include/asm/sighandling.h
> +++ b/arch/x86/include/asm/sighandling.h
> @@ -17,11 +17,11 @@ void signal_fault(struct pt_regs *regs, void __user *frame, char *where);
>
> void __user *
> get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> - void __user **fpstate);
> + void __user **fpstate, u32 pkru);
>
> -int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs);
> -int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> -int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> -int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> +int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
> +int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
> +int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
> +int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
>
> #endif /* _ASM_X86_SIGHANDLING_H */
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 247f2225aa9f..2b3b9e140dd4 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -156,7 +156,7 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
> return !err;
> }
>
> -static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
> +static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf, u32 pkru)
> {
> if (use_xsave())
> return xsave_to_user_sigframe(buf);
> @@ -185,7 +185,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
> * For [f]xsave state, update the SW reserved fields in the [f]xsave frame
> * indicating the absence/presence of the extended state to the user.
> */
> -bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
> +bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size, u32 pkru)
> {
> struct task_struct *tsk = current;
> struct fpstate *fpstate = tsk->thread.fpu.fpstate;
> @@ -228,7 +228,7 @@ bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
> fpregs_restore_userregs();
>
> pagefault_disable();
> - ret = copy_fpregs_to_sigframe(buf_fx);
> + ret = copy_fpregs_to_sigframe(buf_fx, pkru);
> pagefault_enable();
> fpregs_unlock();
>
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 31b6f5dddfc2..94b894437327 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -74,7 +74,7 @@ static inline int is_x32_frame(struct ksignal *ksig)
> */
> void __user *
> get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> - void __user **fpstate)
> + void __user **fpstate, u32 pkru)
we can keep the signature the same, i.e. not adding pkru.
> {
> struct k_sigaction *ka = &ksig->ka;
> int ia32_frame = is_ia32_frame(ksig);
> @@ -139,7 +139,7 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> }
>
> /* save i387 and extended state */
> - if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size))
> + if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru))
> return (void __user *)-1L;
>
> return (void __user *)sp;
> @@ -206,7 +206,7 @@ unsigned long get_sigframe_size(void)
> }
>
> static int
> -setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> +setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru)
> {
> /* Perform fixup for the pre-signal frame. */
> rseq_signal_deliver(ksig, regs);
> @@ -214,21 +214,22 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> /* Set up the stack frame */
> if (is_ia32_frame(ksig)) {
> if (ksig->ka.sa.sa_flags & SA_SIGINFO)
> - return ia32_setup_rt_frame(ksig, regs);
> + return ia32_setup_rt_frame(ksig, regs, pkru);
> else
> - return ia32_setup_frame(ksig, regs);
> + return ia32_setup_frame(ksig, regs, pkru);
> } else if (is_x32_frame(ksig)) {
> - return x32_setup_rt_frame(ksig, regs);
> + return x32_setup_rt_frame(ksig, regs, pkru);
> } else {
> - return x64_setup_rt_frame(ksig, regs);
> + return x64_setup_rt_frame(ksig, regs, pkru);
> }
> }
>
> static void
> handle_signal(struct ksignal *ksig, struct pt_regs *regs)
> {
> - bool stepping, failed;
> struct fpu *fpu = ¤t->thread.fpu;
> + u32 pkru = read_pkru();
This can be moved to get_sigframe(), the same for setting pkru=0
> + bool stepping, failed;
>
> if (v8086_mode(regs))
> save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
> @@ -264,7 +265,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
> if (stepping)
> user_disable_single_step(current);
>
> - failed = (setup_rt_frame(ksig, regs) < 0);
> + failed = (setup_rt_frame(ksig, regs, pkru) < 0);
The failure case can be handled in get_sigframe().
> if (!failed) {
> /*
> * Clear the direction flag as per the ABI for function entry.
> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
> index ef654530bf5a..b437d02ecfd7 100644
> --- a/arch/x86/kernel/signal_32.c
> +++ b/arch/x86/kernel/signal_32.c
no change to signal_64.c if you keep pkru inside get_sigframe.
> @@ -228,7 +228,7 @@ do { \
> goto label; \
> } while(0)
>
> -int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs)
> +int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru)
ia32 doesn't support pkru iiuc, so no need to change the signature here.
Same comments for x32 related code path.
> {
> sigset32_t *set = (sigset32_t *) sigmask_to_save();
> struct sigframe_ia32 __user *frame;
> @@ -246,7 +246,7 @@ int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs)
> 0x80cd, /* int $0x80 */
> };
>
> - frame = get_sigframe(ksig, regs, sizeof(*frame), &fp);
> + frame = get_sigframe(ksig, regs, sizeof(*frame), &fp, pkru);
>
> if (ksig->ka.sa.sa_flags & SA_RESTORER) {
> restorer = ksig->ka.sa.sa_restorer;
> @@ -299,7 +299,7 @@ int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs)
> return -EFAULT;
> }
>
> -int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> +int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru)
> {
> sigset32_t *set = (sigset32_t *) sigmask_to_save();
> struct rt_sigframe_ia32 __user *frame;
> @@ -319,7 +319,7 @@ int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> 0,
> };
>
> - frame = get_sigframe(ksig, regs, sizeof(*frame), &fp);
> + frame = get_sigframe(ksig, regs, sizeof(*frame), &fp, pkru);
>
> if (!user_access_begin(frame, sizeof(*frame)))
> return -EFAULT;
> diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
> index 8a94053c5444..ccfb7824ab2c 100644
> --- a/arch/x86/kernel/signal_64.c
> +++ b/arch/x86/kernel/signal_64.c
no change to signal_64.c if you keep pkru inside get_sigframe.
> @@ -161,7 +161,7 @@ static unsigned long frame_uc_flags(struct pt_regs *regs)
> return flags;
> }
>
> -int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> +int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru)
no change to this function, because it doesn't access altstack.
> {
> sigset_t *set = sigmask_to_save();
> struct rt_sigframe __user *frame;
> @@ -172,7 +172,7 @@ int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
> return -EFAULT;
>
> - frame = get_sigframe(ksig, regs, sizeof(struct rt_sigframe), &fp);
> + frame = get_sigframe(ksig, regs, sizeof(struct rt_sigframe), &fp, pkru);
> uc_flags = frame_uc_flags(regs);
>
> if (!user_access_begin(frame, sizeof(*frame)))
> @@ -300,7 +300,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to,
> return __copy_siginfo_to_user32(to, from);
> }
>
> -int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> +int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru)
> {
> compat_sigset_t *set = (compat_sigset_t *) sigmask_to_save();
> struct rt_sigframe_x32 __user *frame;
> @@ -311,7 +311,7 @@ int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
> return -EFAULT;
>
> - frame = get_sigframe(ksig, regs, sizeof(*frame), &fp);
> + frame = get_sigframe(ksig, regs, sizeof(*frame), &fp, pkru);
>
> uc_flags = frame_uc_flags(regs);
>
> --
> 2.39.3
>
^ permalink raw reply [flat|nested] 3+ messages in thread