Re: [PATCH v2 24/39] x86/cet/shstk: Add user-mode shadow stack support

[Edgecombe, Rick P]

Just now realizing, I never responded to most of this feedback as the
later conversation focused in on one area. All seems good (thanks!),
except not sure about the below:

On Mon, 2022-10-03 at 12:43 -0700, Kees Cook wrote:
> On Thu, Sep 29, 2022 at 03:29:21PM -0700, Rick Edgecombe wrote:
> > +
> > +	mmap_write_lock(mm);
> > +	addr = do_mmap(NULL, addr, size, PROT_READ, flags,
> > +		       VM_SHADOW_STACK | VM_WRITE, 0, &unused, NULL);
> 
> This will use the mmap base address offset randomization, I guess?

Yes.

> 
> > +
> > +	mmap_write_unlock(mm);
> > +
> > +	return addr;
> > +}
> > +
> > +static void unmap_shadow_stack(u64 base, u64 size)
> > +{
> > +	while (1) {
> > +		int r;
> > +
> > +		r = vm_munmap(base, size);
> > +
> > +		/*
> > +		 * 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;
> > +		}
> > +
> > +		/*
> > +		 * For all other types of vm_munmap() failure, either
> > the
> > +		 * system is out of memory or there is bug.
> > +		 */
> > +		WARN_ON_ONCE(r);
> > +		break;
> > +	}
> > +}
> > +
> > +int shstk_setup(void)
> 
> Only called local. Make static?
> 
> > +{
> > +	struct thread_shstk *shstk = &current->thread.shstk;
> > +	unsigned long addr, size;
> > +
> > +	/* Already enabled */
> > +	if (feature_enabled(CET_SHSTK))
> > +		return 0;
> > +
> > +	/* Also not supported for 32 bit */
> > +	if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> > in_ia32_syscall())
> > +		return -EOPNOTSUPP;
> > +
> > +	size = PAGE_ALIGN(min_t(unsigned long long,
> > rlimit(RLIMIT_STACK), SZ_4G));
> > +	addr = alloc_shstk(size);
> > +	if (IS_ERR_VALUE(addr))
> > +		return PTR_ERR((void *)addr);
> > +
> > +	fpu_lock_and_load();
> > +	wrmsrl(MSR_IA32_PL3_SSP, addr + size);
> > +	wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN);
> > +	fpregs_unlock();
> > +
> > +	shstk->base = addr;
> > +	shstk->size = size;
> > +	feature_set(CET_SHSTK);
> > +
> > +	return 0;
> > +}
> > +
> > +void reset_thread_shstk(void)
> > +{
> > +	memset(&current->thread.shstk, 0, sizeof(struct thread_shstk));
> > +	current->thread.features = 0;
> > +	current->thread.features_locked = 0;
> > +}
> 
> If features is always going to be tied to shstk, why not put them in
> the
> shstk struct?

CET and LAM used to share an enabling interface and also kernel side
enablement state tracking. But in the end LAM got its own enabling
interface. Thomas had suggested that they could share a state field on
the kernel side. But then LAM already had enough state tracking for
it's needs.

Shadow stack used to track enabling with the fields in the shstk struct
that keep track of the threads shadow stack. But then we added WRSS
which needs another field to keep track of the status. So I thought to
leave the 'features' field and use it for all the CET features
tracking. I left it outside of the shstk struct so it looks usable for
any other features that might be looking for an status bit. I can
definitely compile it out when there is no user shadow stack.

snip


> > +
> > +void shstk_free(struct task_struct *tsk)
> > +{
> > +	struct thread_shstk *shstk = &tsk->thread.shstk;
> > +
> > +	if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> > +	    !feature_enabled(CET_SHSTK))
> > +		return;
> > +
> > +	if (!tsk->mm)
> > +		return;
> > +
> > +	unmap_shadow_stack(shstk->base, shstk->size);
> 
> I feel like base and size should be zeroed here?
> 

The code used to use shstk->base and shstk->size to keep track of if
shadow stack was enabled. I'm not sure why to zero it now. Just
defensively or did you see a concrete issue?