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?
> +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.
> + 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@xxxxxxxxx
?
> +
> + 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?
> +
> + 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?
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
