Re: [PATCH v2 28/39] x86/cet/shstk: Introduce map_shadow_stack syscall

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, Oct 4, 2022 at 3:56 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Mon, 2022-10-03 at 15:23 -0700, Kees Cook wrote:
> > On Thu, Sep 29, 2022 at 03:29:25PM -0700, Rick Edgecombe wrote:
> > > [...]
> > > The following example demonstrates how to create a new shadow stack
> > > with
> > > map_shadow_stack:
> > > void *shstk = map_shadow_stack(adrr, stack_size,
> > > SHADOW_STACK_SET_TOKEN);
> >
> > typo: addr
>
> Yep, thanks.
>
>
> >
> > > [...]
> > > +451        common  map_shadow_stack        sys_map_shadow_stac
> > > k
> >
> > Isn't this "64", not "common"?
>
> Yes, this should have been changed after dropping 32 bit.

We don't support ia32.  But this is used for x32 which is supported.

> >
> > > [...]
> > > +#define SHADOW_STACK_SET_TOKEN     0x1     /* Set up a restore token
> > > in the shadow stack */
> >
> > I think this should get an intro comment, like:
> >
> > /* Flags for map_shadow_stack(2) */
> >
> > Also, as with the other UAPI fields, please use "(1ULL << 0)" here.
>
> Ok.
>
> >
> > > @@ -62,24 +63,34 @@ static int create_rstor_token(unsigned long
> > > ssp, unsigned long *token_addr)
> > >     if (write_user_shstk_64((u64 __user *)addr, (u64)ssp))
> > >             return -EFAULT;
> > >
> > > -   *token_addr = addr;
> > > +   if (token_addr)
> > > +           *token_addr = addr;
> > >
> > >     return 0;
> > >  }
> > >
> >
> > Can this just be collapsed into the patch that introduces
> > create_rstor_token()?
>
> I mean, yea, that would be simpler. Breaking the changes apart was left
> over from when the signals placed a token, but didn't need this extra
> bit of functionality.
>
> >
> > > -static unsigned long alloc_shstk(unsigned long size)
> > > +static unsigned long alloc_shstk(unsigned long addr, unsigned long
> > > size,
> > > +                            unsigned long token_offset, bool
> > > set_res_tok)
> > >  {
> > >     int flags = MAP_ANONYMOUS | MAP_PRIVATE;
> > >     struct mm_struct *mm = current->mm;
> > > -   unsigned long addr, unused;
> > > +   unsigned long mapped_addr, unused;
> > >
> > >     mmap_write_lock(mm);
> > > -   addr = do_mmap(NULL, addr, size, PROT_READ, flags,
> >
> > Oops, I missed in the other patch that "addr" was being passed here.
> > (uninitialized?)
>
> Argh, yes. I'll initialize in that patch and remove it here.
>
> >
> > > -                  VM_SHADOW_STACK | VM_WRITE, 0, &unused, NULL);
> > > -
> > > +   mapped_addr = do_mmap(NULL, addr, size, PROT_READ, flags,
> > > +                         VM_SHADOW_STACK | VM_WRITE, 0, &unused,
> > > NULL);
> >
> > I don't see do_mmap() doing anything here to avoid remapping a prior
> > vma
> > as shstk. Is the intention to allow userspace to convert existing
> > VMAs?
> > This has caused pain in the past, perhaps force MAP_FIXED_NOREPLACE ?
>
> No that is not the intention. It should fail and MAP_FIXED_NOREPLACE
> looks like it will fit the bill. Thanks!
>
> >
> > > [...]
> > > @@ -174,6 +185,7 @@ int shstk_alloc_thread_stack(struct task_struct
> > > *tsk, unsigned long clone_flags,
> > >
> > >
> > >     stack_size = PAGE_ALIGN(stack_size);
> > > +   addr = alloc_shstk(0, stack_size, 0, false);
> > >     if (IS_ERR_VALUE(addr))
> > >             return PTR_ERR((void *)addr);
> > >
> >
> > As mentioned earlier, I was expecting this patch to replace a
> > (missing)
> > call to alloc_shstk. i.e. expecting:
> >
> > -     addr = alloc_shstk(stack_size);
> >
> > > @@ -395,6 +407,26 @@ int shstk_disable(void)
> > >     return 0;
> > >  }
> > >
> > > +
> > > +SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned
> > > long, size, unsigned int, flags)
> >
> > Please add kern-doc for this, with some notes. E.g. at least one
> > thing isn't immediately
> > obvious, maybe more: "addr" must be a multiple of 8.
>
> Ok.
>
> >
> > > +{
> > > +   unsigned long aligned_size;
> > > +
> > > +   if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > > +           return -ENOSYS;
> >
> > This needs to explicitly reject unknown flags[1], or expanding them
> > in the
> > future becomes very painful:
> >
> >       if (flags & ~(SHADOW_STACK_SET_TOKEN))
> >               return -EINVAL;
> >
> >
> > [1]
> > https://docs.kernel.org/process/adding-syscalls.html#designing-the-api-planning-for-extension
> >
>
> Ok, good idea.
>
> > > +
> > > +   /*
> > > +    * An overflow would result in attempting to write the restore
> > > token
> > > +    * to the wrong location. Not catastrophic, but just return the
> > > right
> > > +    * error code and block it.
> > > +    */
> > > +   aligned_size = PAGE_ALIGN(size);
> > > +   if (aligned_size < size)
> > > +           return -EOVERFLOW;
> >
> > The intention here is to allow userspace to ask for _less_ than a
> > page
> > size multiple, and to put the restore token there?
> >
> > Is it worth adding a check for size >= 8 here? Or, I guess it would
> > just
> > immediately crash on the next call?
>
> Funny you should ask... The glibc changes were doing this and then
> looking for the token at the end of the length that it passed (not the
> page aligned length). I had changed the kernel at one point to be page
> aligned and then had the fun of debugging the results. I thought, glibc
>  is just wasting shadow stack. It should ask for page aligned shadow
> stacks. But HJ argued that the kernel shouldn't second guess what
> userspace is asking for based on HW page size details that don't have
> to do with the software interface. I was convinced by that argument,
> even though glibc is still wasting space.
>
> I could still be convinced the other way though. Glibc still has time
> to (and should) change. But yea, that was actually the intention.

Glibc requests a shadow stack of a given size and expects the restore
token at the specific location.  This is how glibc uses the restore token
to switch to the new shadow stack.

> >
> > > +
> > > +   return alloc_shstk(addr, aligned_size, size, flags &
> > > SHADOW_STACK_SET_TOKEN);
> > > +}
> >
> >



-- 
H.J.