Re: [PATCH v6 0/5] x86/pkeys: update PKRU to enable pkey 0 before

Re: [PATCH v6 0/5] x86/pkeys: update PKRU to enable pkey 0 before

[Jeff Xu]

Hi Aruna,

I added Rick who commented on the previous version of patch. I added
Kees/Jorge/Stephan/Yann,  they expressed interest in similar
functionality before. It would be best to keep them in future versions
of this patch.

I also add linux-mm for more reviews/feedback on signal handling path.

On Thu, Jun 27, 2024 at 2:17 PM Aruna Ramakrishna
<aruna.ramakrishna@oracle.com> wrote:
>
> v6 updates:
> - Rebased patchset to v6.10.0-rc5
> - Changed sig_prepare_pkru() back to enabling all pkeys, based on
> discussion with Jeff Xu
Thanks for taking the suggestion.

>
> As Jeff mentioned, I'm unsure if we need to retain the changes to the
> 32-bit functions. Maintainers - please advise.
>
It is possible to  reduce the size of change.
Not touching the ia32 or x32 code path also means less test in
multiple architectures and less risk of regression.
I will add comments in the specific  patch to clarify what I meant,
please give it a try to see if it makes sense.

-Jeff





-Jeff



> v5 updates:
> - No major changes, mostly a resend of v4 - except for updating the
>   commit description for patch 5/5
>
> v4 updates (based on review feedback from Thomas Gleixner):
> - Simplified update_pkru_in_sigframe()
> - Changed sigpkru to enable minimally required keys (init_pkru and
>   current
>   pkru)
> - Modified pkey_sighandler_tests.c to use kselfttest framework
> - Fixed commit descriptions
> - Fixed sigreturn use case (pointed out by Jeff Xu)
> - Added a new sigreturn test case
>
> v3 updates (based on review feedback from Ingo Molnar and Dave Hansen):
> - Split the original patch into 3:
>         - function interface changes
>         - helper functions
>         - functional change to write pkru on sigframe
> - Enabled all pkeys before XSAVE - i.e. wrpkru(0), rather than assuming
> that the alt sig stack is always protected by pkey 0.
> - Added a few test cases in pkey_sighandler_tests.c.
>
> I had some trouble adding these tests to
> tools/testing/selftests/mm/protection_keys.c, so they're in a separate
> file.
>
> Aruna Ramakrishna (4):
>   x86/pkeys: Add PKRU as a parameter in signal handling functions
>   x86/pkeys: Add helper functions to update PKRU on the sigframe
>   x86/pkeys: Update PKRU to enable all pkeys before XSAVE
>   x86/pkeys: Restore altstack before sigcontext
>
> Keith Lucas (1):
>   selftests/mm: Add new testcases for pkeys
>
>  arch/x86/include/asm/fpu/signal.h             |   2 +-
>  arch/x86/include/asm/sighandling.h            |  10 +-
>  arch/x86/kernel/fpu/signal.c                  |  27 +-
>  arch/x86/kernel/fpu/xstate.c                  |  13 +
>  arch/x86/kernel/fpu/xstate.h                  |   2 +
>  arch/x86/kernel/signal.c                      |  45 +-
>  arch/x86/kernel/signal_32.c                   |  12 +-
>  arch/x86/kernel/signal_64.c                   |  14 +-
>  tools/testing/selftests/mm/Makefile           |   2 +
>  tools/testing/selftests/mm/pkey-helpers.h     |  11 +-
>  .../selftests/mm/pkey_sighandler_tests.c      | 479 ++++++++++++++++++
>  tools/testing/selftests/mm/protection_keys.c  |  10 -
>  12 files changed, 583 insertions(+), 44 deletions(-)
>  create mode 100644 tools/testing/selftests/mm/pkey_sighandler_tests.c
>
>
> base-commit: f2661062f16b2de5d7b6a5c42a9a5c96326b8454
> prerequisite-patch-id: fc7cbe9ff3c554d556e2792f9ffaf29e6ad6ee21
> --
> 2.39.3
>

Re: [PATCH v6 1/5] x86/pkeys: Add PKRU as a parameter in signal handling functions

[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 = &current->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
>

Re: [PATCH v6 0/5] x86/pkeys: update PKRU to enable pkey 0 before

[Aruna Ramakrishna]

> On Jul 16, 2024, at 9:25 PM, Jeff Xu <jeffxu@chromium.org> wrote:
> 
> Hi Aruna,
> 
> I added Rick who commented on the previous version of patch. I added
> Kees/Jorge/Stephan/Yann,  they expressed interest in similar
> functionality before. It would be best to keep them in future versions
> of this patch.
> 
> I also add linux-mm for more reviews/feedback on signal handling path.
> 
> On Thu, Jun 27, 2024 at 2:17 PM Aruna Ramakrishna
> <aruna.ramakrishna@oracle.com> wrote:
>> 
>> v6 updates:
>> - Rebased patchset to v6.10.0-rc5
>> - Changed sig_prepare_pkru() back to enabling all pkeys, based on
>> discussion with Jeff Xu
> Thanks for taking the suggestion.
> 
>> 
>> As Jeff mentioned, I'm unsure if we need to retain the changes to the
>> 32-bit functions. Maintainers - please advise.
>> 
> It is possible to  reduce the size of change.
> Not touching the ia32 or x32 code path also means less test in
> multiple architectures and less risk of regression.

Apologies for the delay in responding - I was on vacation and got back
yesterday.

I agree, I’m unsure if 32-bit support is needed here - as you pointed out in an
earlier email, the man page says:
“The feature is only available in 64-bit mode, even though there is theoretically
space in the PAE PTEs.”

I was hoping that Dave/Thomas would review and clarify. I will remove the
32-bit support from this patchset and send out a v7.

Thanks,
Aruna