Messages in this thread | | | Subject | Re: [PATCH v26 23/30] x86/cet/shstk: Handle thread shadow stack | From | "Yu, Yu-cheng" <> | Date | Mon, 10 May 2021 15:57:56 -0700 |
| |
On 5/10/2021 7:15 AM, Borislav Petkov wrote: > On Tue, Apr 27, 2021 at 01:43:08PM -0700, Yu-cheng Yu wrote:
[...]
>> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c >> index c815c7507830..d387df84b7f1 100644 >> --- a/arch/x86/kernel/shstk.c >> +++ b/arch/x86/kernel/shstk.c >> @@ -70,6 +70,55 @@ int shstk_setup(void) >> return 0; >> } > >> +int shstk_setup_thread(struct task_struct *tsk, unsigned long clone_flags, > > Judging by what this function does, its name wants to be > > shstk_alloc_thread_stack() > > or so? > >> + unsigned long stack_size) >> +{ >> + unsigned long addr, size; >> + struct cet_user_state *state; >> + struct cet_status *cet = &tsk->thread.cet; > > The tip-tree preferred ordering of variable declarations at the > beginning of a function is reverse fir tree order:: > > struct long_struct_name *descriptive_name; > unsigned long foo, bar; > unsigned int tmp; > int ret; > > The above is faster to parse than the reverse ordering:: > > int ret; > unsigned int tmp; > unsigned long foo, bar; > struct long_struct_name *descriptive_name; > > And even more so than random ordering:: > > unsigned long foo, bar; > int ret; > struct long_struct_name *descriptive_name; > unsigned int tmp; > >> + >> + if (!cet->shstk_size) >> + return 0; >> + > > This check needs a comment. > >> + if ((clone_flags & (CLONE_VFORK | CLONE_VM)) != CLONE_VM) >> + return 0; >> + >> + state = get_xsave_addr(&tsk->thread.fpu.state.xsave, >> + XFEATURE_CET_USER); > > Let that line stick out. > >> + >> + if (!state) >> + return -EINVAL; >> + >> + if (stack_size == 0) > > if (!stack_size) > >> + return -EINVAL; > > and that test needs to be done first in the function. > >> + >> + /* Cap shadow stack size to 4 GB */ > > Why? >
This is not necessary. I will make it just stack_size, which is passed in from copy_thread().
>> + size = min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G); >> + size = min(size, stack_size); >> + >> + /* >> + * Compat-mode pthreads share a limited address space. >> + * If each function call takes an average of four slots >> + * stack space, allocate 1/4 of stack size for shadow stack. >> + */ >> + if (in_compat_syscall()) >> + size /= 4; > > <---- newline here. > >> + size = round_up(size, PAGE_SIZE); >> + addr = alloc_shstk(size); >> + > > ^ Superfluous newline. > >> + if (IS_ERR_VALUE(addr)) { >> + cet->shstk_base = 0; >> + cet->shstk_size = 0; >> + return PTR_ERR((void *)addr); >> + } >> + >> + fpu__prepare_write(&tsk->thread.fpu); >> + state->user_ssp = (u64)(addr + size); > > cet_user_state has u64, cet_status has unsigned longs. Make them all u64. > > And since cet_status is per thread, but I had suggested struct > shstk_desc, I think now that that should be called > > struct thread_shstk > > or so to denote *exactly* what it is. >
So this struct will be:
struct thread_shstk { u64 shstk_base; u64 shstk_size; u64 locked:1; u64 ibt:1; };
Ok?
Thanks, Yu-cheng
| |