Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()

[Christian Brauner <brauner@kernel.org>]

On Wed, Aug 21, 2024 at 06:23:18PM GMT, Mark Brown wrote:
> On Wed, Aug 21, 2024 at 03:54:49PM +0000, Edgecombe, Rick P wrote:
> > On Wed, 2024-08-21 at 13:45 +0100, Mark Brown wrote:
> 
> > > It's entirely possible it just leaked.  My own attempts to dig through
> > > the archives haven't turned up anything on the subjecti either, it seems
> > > to have been there from the get go and just gone in without comment.
> > > Equally it could just be that people felt that this was a more tasteful
> > > way of specifying stacks, or that some future use was envisioned.
> 
> > Ok, well I'm suspicious, but won't object over it. The rest seems settled from
> > my side. I may try to attract some other x86 attention to that CMPXCHG helper,
> > but otherwise.
> 
> OK, I'll post what I've got (with the current ABI) today, incorporating
> your x86 fixes and the tighter validation and we can see what people
> think.  Perhaps Christian remembers what's going on there?

The legacy clone system call had required userspace to know in which
direction the stack was growing and then pass down the stack pointer
appropriately (e.g., parisc grows upwards).

And in fact, the old clone() system call did take an additional
stack_size argument on specific architectures. For example, on
microblaze.

Also, when clone3() was done we still had ia64 in the tree which had a
separate clone2() system call that also required a stack_size argument.

So userspace ended up with code like this or worse:

     #define __STACK_SIZE (8 * 1024 * 1024)
     pid_t sys_clone(int (*fn)(void *), void *arg, int flags, int *pidfd)
     {
             pid_t ret;
             void *stack;

             stack = malloc(__STACK_SIZE);
             if (!stack)
                     return -ENOMEM;

     #ifdef __ia64__
             ret = __clone2(fn, stack, __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
     #elif defined(__parisc__) /* stack grows up */
             ret = clone(fn, stack, flags | SIGCHLD, arg, pidfd);
     #else
             ret = clone(fn, stack + __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
     #endif
             return ret;
     }

So we talked to the glibc folks which preferred the kernel to do all
this nonsense for them as it has that knowledge.

My preference is to keep the api consistent and require a stack_size for
shadow stacks as well.