Re: [PATCH v28 25/32] x86/cet/shstk: Handle thread shadow stack

[Dave Hansen]

On 7/22/21 1:52 PM, Yu-cheng Yu wrote:
> +	if (!stack_size)
> +		stack_size = min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G);
> +
> +	if (!shstk->size)
> +		return 0;
> +
> +	/*
> +	 * For CLONE_VM, except vfork, the child needs a separate shadow
> +	 * stack.
> +	 */
> +	if ((clone_flags & (CLONE_VFORK | CLONE_VM)) != CLONE_VM)
> +		return 0;
> +
> +	/*
> +	 * This is in clone() syscall and fpu__copy() already copies xstates
> +	 * from the parent.  If get_xsave_addr() returns null, then XFEATURE_
> +	 * CET_USER is still in init state, which certainly is an error.
> +	 */
> +	state = get_xsave_addr(&tsk->thread.fpu.state.xsave, XFEATURE_CET_USER);
> +	if (!state)
> +		return -EINVAL;

I don't care much for that comment.

This code is meant to copy shadow stack config information into children
when it is already enabled.  We *just* checked for that above in the
"shstk->size" check.  The fact that this is called from clone() is
irrelevant.  The shadow stack enabling status *is*.

I think I'd rather this be more along the lines of:

	/*
	 * 'tsk' is configured with a shadow stack and the fpu.state is
	 * up to date since it was just copied from the parent.  There
	 * must be a valid non-init CET state location in the buffer.
	 */

There is also a strong enough assumption violation that I'd probably
WARN() in addition to returning -EINVAL.