Re: [PATCH v23 22/28] x86/cet/shstk: User-mode shadow stack support

[Yu, Yu-cheng]

On 3/18/2021 5:32 AM, Borislav Petkov wrote:
> > Subject: Re: [PATCH v23 22/28] x86/cet/shstk:   User-mode shadow stack support
> 						^
> 						Add
>
> On Tue, Mar 16, 2021 at 08:10:48AM -0700, Yu-cheng Yu wrote:
> > Introduce basic shadow stack enabling/disabling/allocation routines.
> > A task's shadow stack is allocated from memory with VM_SHSTK flag and has
> > a fixed size of min(RLIMIT_STACK, 4GB).
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> >   arch/x86/include/asm/cet.h       |  28 ++++++
> >   arch/x86/include/asm/processor.h |   5 ++
> >   arch/x86/kernel/Makefile         |   2 +
> >   arch/x86/kernel/cet.c            | 147 +++++++++++++++++++++++++++++++

[...]

> > +void cet_free_shstk(struct task_struct *tsk)
> > +{
> > +	struct cet_status *cet = &tsk->thread.cet;
> > +
> > +	if (!static_cpu_has(X86_FEATURE_SHSTK) ||
>
> cpu_feature_enabled and as above.
>
> > +	    !cet->shstk_size || !cet->shstk_base)
> > +		return;
> > +
> > +	if (!tsk->mm || tsk->mm != current->mm)
> > +		return;
>
> You're operating on current here merrily but what's protecting all those
> paths operating on current from getting current changed underneath them
> due to scheduling? IOW, is preemption safely disabled in all those
> paths ending up here?

Good thought.  Indeed, this looks like scheduling would bring some 
trouble.  However, when this instance is running, the current task must 
be current, context switch or not.  The purpose of this check is 
described below.

When fork() fails, it calls exit_thread(), then cet_free_shstk(). 
Normally the child tsk->mm != current->mm (parent).  There is no need to 
free shadow stack.

For CLONE_VM, however, the kernel has already allocated a shadow stack 
for the child and needs to free it because fork() failed.

Maybe I would add comments here.

> > +
> > +	while (1) {
>
> Uuh, an endless loop. What guarantees we'll exit it relatively timely...
>
> > +		int r;
> > +
> > +		r = vm_munmap(cet->shstk_base, cet->shstk_size);
> > +
> > +		/*
> > +		 * Retry if mmap_lock is not available.
> > +		 */
> > +		if (r == -EINTR) {
> > +			cond_resched();
>
> ... that thing?

If vm_munmap() returns -EINTR, mmap_lock is held by something else. 
That lock should not be held forever.  For other types of error, the 
loop stops.

> > +			continue;
> > +		}
> > +
> > +		WARN_ON_ONCE(r);
> > +		break;
> > +	}
> > +
> > +	cet->shstk_base = 0;
> > +	cet->shstk_size = 0;
> > +}
> > --
> > 2.21.0