On 7/22/2021 2:05 PM, Dave Hansen wrote:

On 7/22/21 1:52 PM, Yu-cheng Yu wrote:

quoted

+	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.

Yes, I will update the comment and put in the WARN().

Thanks,
Yu-cheng