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

Jann Horn
2018-10-03T16:47:14Z

On Fri, Sep 21, 2018 at 5:09 PM Yu-cheng Yu <yu-cheng.yu@intel.com> 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;
> +}