Messages in this thread | | | Subject | Re: [PATCH v28 25/32] x86/cet/shstk: Handle thread shadow stack | From | "Yu, Yu-cheng" <> | Date | Fri, 23 Jul 2021 10:30:26 -0700 |
| |
On 7/22/2021 2:05 PM, Dave Hansen wrote: > 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. >
Yes, I will update the comment and put in the WARN().
Thanks, Yu-cheng
| |