lkml.org 
[lkml]   [2022]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 27/39] x86/cet/shstk: Handle signals for shadow stack
    On Thu, Sep 29, 2022 at 03:29:24PM -0700, Rick Edgecombe wrote:
    > From: Yu-cheng Yu <yu-cheng.yu@intel.com>
    >
    > When a signal is handled normally the context is pushed to the stack
    > before handling it. For shadow stacks, since the shadow stack only track's
    > return addresses, there isn't any state that needs to be pushed. However,
    > there are still a few things that need to be done. These things are
    > userspace visible and which will be kernel ABI for shadow stacks.
    >
    > One is to make sure the restorer address is written to shadow stack, since
    > the signal handler (if not changing ucontext) returns to the restorer, and
    > the restorer calls sigreturn. So add the restorer on the shadow stack
    > before handling the signal, so there is not a conflict when the signal
    > handler returns to the restorer.
    >
    > The other thing to do is to place some type of checkable token on the
    > thread's shadow stack before handling the signal and check it during
    > sigreturn. This is an extra layer of protection to hamper attackers
    > calling sigreturn manually as in SROP-like attacks.
    >
    > For this token we can use the shadow stack data format defined earlier.
    > Have the data pushed be the previous SSP. In the future the sigreturn
    > might want to return back to a different stack. Storing the SSP (instead
    > of a restore offset or something) allows for future functionality that
    > may want to restore to a different stack.
    >
    > So, when handling a signal push
    > - the SSP pointing in the shadow stack data format
    > - the restorer address below the restore token.
    >
    > In sigreturn, verify SSP is stored in the data format and pop the shadow
    > stack.
    >
    > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
    > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
    > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
    > Cc: Andy Lutomirski <luto@kernel.org>
    > Cc: Cyrill Gorcunov <gorcunov@gmail.com>
    > Cc: Florian Weimer <fweimer@redhat.com>
    > Cc: H. Peter Anvin <hpa@zytor.com>
    > Cc: Kees Cook <keescook@chromium.org>
    >
    > ---
    >
    > v2:
    > - Switch to new shstk signal format
    >
    > v1:
    > - Use xsave helpers.
    > - Expand commit log.
    >
    > Yu-cheng v27:
    > - Eliminate saving shadow stack pointer to signal context.
    >
    > Yu-cheng v25:
    > - Update commit log/comments for the sc_ext struct.
    > - Use restorer address already calculated.
    > - Change CONFIG_X86_CET to CONFIG_X86_SHADOW_STACK.
    > - Change X86_FEATURE_CET to X86_FEATURE_SHSTK.
    > - Eliminate writing to MSR_IA32_U_CET for shadow stack.
    > - Change wrmsrl() to wrmsrl_safe() and handle error.
    >
    > arch/x86/ia32/ia32_signal.c | 1 +
    > arch/x86/include/asm/cet.h | 5 ++
    > arch/x86/kernel/shstk.c | 126 ++++++++++++++++++++++++++++++------
    > arch/x86/kernel/signal.c | 10 +++
    > 4 files changed, 123 insertions(+), 19 deletions(-)
    >
    > diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
    > index c9c3859322fa..88d71b9de616 100644
    > --- a/arch/x86/ia32/ia32_signal.c
    > +++ b/arch/x86/ia32/ia32_signal.c
    > @@ -34,6 +34,7 @@
    > #include <asm/sigframe.h>
    > #include <asm/sighandling.h>
    > #include <asm/smap.h>
    > +#include <asm/cet.h>
    >
    > static inline void reload_segments(struct sigcontext_32 *sc)
    > {
    > diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
    > index 924de99e0c61..8c6fab9f402a 100644
    > --- a/arch/x86/include/asm/cet.h
    > +++ b/arch/x86/include/asm/cet.h
    > @@ -6,6 +6,7 @@
    > #include <linux/types.h>
    >
    > struct task_struct;
    > +struct ksignal;
    >
    > struct thread_shstk {
    > u64 base;
    > @@ -22,6 +23,8 @@ int shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags,
    > void shstk_free(struct task_struct *p);
    > int shstk_disable(void);
    > void reset_thread_shstk(void);
    > +int setup_signal_shadow_stack(struct ksignal *ksig);
    > +int restore_signal_shadow_stack(void);
    > #else
    > static inline long cet_prctl(struct task_struct *task, int option,
    > unsigned long features) { return -EINVAL; }
    > @@ -33,6 +36,8 @@ static inline int shstk_alloc_thread_stack(struct task_struct *p,
    > static inline void shstk_free(struct task_struct *p) {}
    > static inline int shstk_disable(void) { return -EOPNOTSUPP; }
    > static inline void reset_thread_shstk(void) {}
    > +static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
    > +static inline int restore_signal_shadow_stack(void) { return 0; }
    > #endif /* CONFIG_X86_SHADOW_STACK */
    >
    > #endif /* __ASSEMBLY__ */
    > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
    > index 8904aef487bf..04442134aadd 100644
    > --- a/arch/x86/kernel/shstk.c
    > +++ b/arch/x86/kernel/shstk.c
    > @@ -227,41 +227,129 @@ static int get_shstk_data(unsigned long *data, unsigned long __user *addr)
    > }
    >
    > /*
    > - * Verify the user shadow stack has a valid token on it, and then set
    > - * *new_ssp according to the token.
    > + * Create a restore token on shadow stack, and then push the user-mode
    > + * function return address.
    > */
    > -static int shstk_check_rstor_token(unsigned long *new_ssp)
    > +static int shstk_setup_rstor_token(unsigned long ret_addr, unsigned long *new_ssp)

    Oh, hrm. Prior patch defines shstk_check_rstor_token() and
    doesn't call it. This patch removes it. :P Can you please remove
    shstk_check_rstor_token() from the prior patch?

    > {
    > - unsigned long token_addr;
    > - unsigned long token;
    > + unsigned long ssp, token_addr;
    > + int err;
    > +
    > + if (!ret_addr)
    > + return -EINVAL;
    > +
    > + ssp = get_user_shstk_addr();
    > + if (!ssp)
    > + return -EINVAL;
    > +
    > + err = create_rstor_token(ssp, &token_addr);
    > + if (err)
    > + return err;
    > +
    > + ssp = token_addr - sizeof(u64);
    > + err = write_user_shstk_64((u64 __user *)ssp, (u64)ret_addr);
    > +
    > + if (!err)
    > + *new_ssp = ssp;
    > +
    > + return err;
    > +}
    > +
    > +static int shstk_push_sigframe(unsigned long *ssp)
    > +{
    > + unsigned long target_ssp = *ssp;
    > +
    > + /* Token must be aligned */
    > + if (!IS_ALIGNED(*ssp, 8))
    > + return -EINVAL;
    >
    > - token_addr = get_user_shstk_addr();
    > - if (!token_addr)
    > + if (!IS_ALIGNED(target_ssp, 8))
    > return -EINVAL;
    >
    > - if (get_user(token, (unsigned long __user *)token_addr))
    > + *ssp -= SS_FRAME_SIZE;
    > + if (put_shstk_data((void *__user)*ssp, target_ssp))
    > return -EFAULT;
    >
    > - /* Is mode flag correct? */
    > - if (!(token & BIT(0)))
    > + return 0;
    > +}
    > +
    > +
    > +static int shstk_pop_sigframe(unsigned long *ssp)
    > +{
    > + unsigned long token_addr;
    > + int err;
    > +
    > + err = get_shstk_data(&token_addr, (unsigned long __user *)*ssp);
    > + if (unlikely(err))
    > + return err;
    > +
    > + /* Restore SSP aligned? */
    > + if (unlikely(!IS_ALIGNED(token_addr, 8)))
    > return -EINVAL;

    Why doesn't this always fail, given BIT(0) being set? I don't see it
    getting cleared until the end of this function.

    >
    > - /* Is busy flag set? */
    > - if (token & BIT(1))
    > + /* SSP in userspace? */
    > + if (unlikely(token_addr >= TASK_SIZE_MAX))
    > return -EINVAL;

    BIT(63) already got cleared by here (in get_shstk_data(), but yes,
    this is still a reasonable check.

    >
    > - /* Mask out flags */
    > - token &= ~3UL;
    > + *ssp = token_addr;
    > +
    > + return 0;
    > +}

    --
    Kees Cook

    \
     
     \ /
      Last update: 2022-10-03 22:53    [W:4.055 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site