Re: [RFC PATCH v4 20/27] x86/cet/shstk: Signal handling for shadow stack

[Jann Horn]

On Fri, Sep 21, 2018 at 5:09 PM Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> wrote:
> When setting up a signal, the kernel creates a shadow stack
> restore token at the current SHSTK address and then stores the
> token's address in the signal frame, right after the FPU state.
> Before restoring a signal, the kernel verifies and then uses the
> restore token to set the SHSTK pointer.
[...]
> +#ifdef CONFIG_X86_64
> +static int copy_ext_from_user(struct sc_ext *ext, void __user *fpu)
> +{
> +       void __user *p;
> +
> +       if (!fpu)
> +               return -EINVAL;
> +
> +       p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
> +       p = (void __user *)ALIGN((unsigned long)p, 8);
> +
> +       if (!access_ok(VERIFY_READ, p, sizeof(*ext)))
> +               return -EFAULT;
> +
> +       if (__copy_from_user(ext, p, sizeof(*ext)))
> +               return -EFAULT;

Why do you first manually call access_ok(), then call
__copy_from_user() with the same size? Just use "if
(copy_from_user(ext, p, sizeof(*ext)))" (without underscores) and get
rid of the access_ok().

> +       if (ext->total_size != sizeof(*ext))
> +               return -EINVAL;
> +       return 0;
> +}
> +
> +static int copy_ext_to_user(void __user *fpu, struct sc_ext *ext)
> +{
> +       void __user *p;
> +
> +       if (!fpu)
> +               return -EINVAL;
> +
> +       if (ext->total_size != sizeof(*ext))
> +               return -EINVAL;
> +
> +       p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
> +       p = (void __user *)ALIGN((unsigned long)p, 8);
> +
> +       if (!access_ok(VERIFY_WRITE, p, sizeof(*ext)))
> +               return -EFAULT;
> +
> +       if (__copy_to_user(p, ext, sizeof(*ext)))
> +               return -EFAULT;

Same as above.

> +       return 0;
> +}