Messages in this thread | | | Subject | Re: [PATCH v26 22/30] x86/cet/shstk: Add user-mode shadow stack support | From | "Yu, Yu-cheng" <> | Date | Wed, 28 Apr 2021 11:39:00 -0700 |
| |
On 4/28/2021 10:52 AM, Borislav Petkov wrote: > On Tue, Apr 27, 2021 at 01:43:07PM -0700, Yu-cheng Yu wrote: >> @@ -535,6 +536,10 @@ struct thread_struct { >> >> unsigned int sig_on_uaccess_err:1; >> >> +#ifdef CONFIG_X86_SHADOW_STACK >> + struct cet_status cet; > > A couple of versions ago I said: > > " struct shstk_desc shstk; > > or so" > > but no movement here. That thing is still called cet_status even though > there's nothing status-related with it. > > So what's up? >
Sorry about that. After that email thread, we went ahead to separate shadow stack and ibt into different files. I thought about the struct, the file names cet.h, etc. The struct still needs to include ibt status, and if it is shstk_desc, the name is not entirely true. One possible approach is, we don't make it a struct here, and put every item directly in thread_struct. However, the benefit of putting all in a struct is understandable (you might argue the opposite :-)). Please make the call, and I will do the change.
>> +static unsigned long alloc_shstk(unsigned long size) >> +{ >> + struct mm_struct *mm = current->mm; >> + unsigned long addr, populate; >> + int flags = MAP_ANONYMOUS | MAP_PRIVATE; > > 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; > > Please fix it up everywhere. >
Ok!
>> + mmap_write_lock(mm); >> + addr = do_mmap(NULL, 0, size, PROT_READ, flags, VM_SHADOW_STACK, 0, >> + &populate, NULL); >> + mmap_write_unlock(mm); >> + >> + return addr; >> +} >> + >> +int shstk_setup(void) >> +{ >> + unsigned long addr, size; >> + struct cet_status *cet = ¤t->thread.cet; >> + >> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) >> + return -EOPNOTSUPP; >> + >> + size = round_up(min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G), PAGE_SIZE); >> + addr = alloc_shstk(size); >> + if (IS_ERR_VALUE(addr)) >> + return PTR_ERR((void *)addr); >> + >> + cet->shstk_base = addr; >> + cet->shstk_size = size; >> + >> + start_update_msrs(); >> + wrmsrl(MSR_IA32_PL3_SSP, addr + size); >> + wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN); >> + end_update_msrs(); > > <---- newline here. > >> + return 0; >> +} >> + >> +void shstk_free(struct task_struct *tsk) >> +{ >> + struct cet_status *cet = &tsk->thread.cet; >> + >> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) || >> + !cet->shstk_size || >> + !cet->shstk_base) >> + return; >> + >> + if (!tsk->mm) >> + return; > > Where are the comments you said you wanna add: > > https://lkml.kernel.org/r/b05ee7eb-1b5d-f84f-c8f3-bfe9426e8a7d@intel.com > > ? >
Yes, the comments are in patch #23: Handle thread shadow stack. I wanted to add that in the patch that takes the path.
>> + >> + while (1) { >> + int r; >> + >> + r = vm_munmap(cet->shstk_base, cet->shstk_size); > > int r = vm_munmap... > >> + >> + /* >> + * vm_munmap() returns -EINTR when mmap_lock is held by >> + * something else, and that lock should not be held for a >> + * long time. Retry it for the case. >> + */ >> + if (r == -EINTR) { >> + cond_resched(); >> + continue; >> + } >> + break; >> + } > > vm_munmap() can return other negative error values, where are you > handling those? >
For other error values, the loop stops.
>> + >> + cet->shstk_base = 0; >> + cet->shstk_size = 0; >> +} >> + >> +void shstk_disable(void) >> +{ >> + struct cet_status *cet = ¤t->thread.cet; > > Same question as before: what guarantees that current doesn't change > from under you here?
The actual reading/writing MSRs are protected by fpregs_lock().
> > One of the worst thing to do is to ignore review comments. I'd strongly > suggest you pay more attention and avoid that in the future. > > Thx. >
| |