Re: [PATCH 1/2] x86/shstk: free shadow stack when copy_thread() fails

[Edgecombe, Rick P]

By inspection the bug looks real to me.

On Wed, 2026-10-14 at 23:31 -0300, Iván Ezequiel Rodriguez wrote:
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 346c438ac880..be840652d055 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -265,11 +265,19 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>  	/* Set a new TLS for the child thread? */
>  	if (clone_flags & CLONE_SETTLS)
>  		ret = set_new_tls(p, tls);
> +	if (ret) {

Having the error check happen even if !CLONE_SETTLS makes it not clear that this
condition could only happen that way. I'd maybe nest the check inside, like is
done in several cases in in copy_process(). Like (untested):

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 346c438ac8801..e40f83a02fdb1 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -176,7 +176,7 @@ int copy_thread(struct task_struct *p, const struct
kernel_clone_args *args)
        struct fork_frame *fork_frame;
        struct pt_regs *childregs;
        unsigned long new_ssp;
-       int ret = 0;
+       int ret;
 
        childregs = task_pt_regs(p);
        fork_frame = container_of(childregs, struct fork_frame, regs);
@@ -263,13 +263,19 @@ int copy_thread(struct task_struct *p, const struct
kernel_clone_args *args)
        }
 
        /* Set a new TLS for the child thread? */
-       if (clone_flags & CLONE_SETTLS)
+       if (clone_flags & CLONE_SETTLS) {
                ret = set_new_tls(p, tls);
+               if (ret)
+                       goto shstk_free;
+       }
 
-       if (!ret && unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
+       if (unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
                io_bitmap_share(p);
 
        return ret;
+shstk_free:
+       shstk_free(p);
+       return ret;
 }
 
 static void pkru_flush_thread(void)



> +		/*
> +		 * copy_process() skips exit_thread() when copy_thread()
> +		 * fails. Free the shadow stack mapped before this error.
> +		 */

I'm not in favor of this comment. It is describing behavior far away. And the
idea that you cleanup what you allocated in on a later error condition is a
pretty common pattern. I tried to write something and nothing seems non-obvious.
Maybe just drop it.

> +		shstk_free(p);
> +		return ret;
> +	}
>  
> -	if (!ret && unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
> +	if (unlikely(test_tsk_thread_flag(current, TIF_IO_BITMAP)))
>  		io_bitmap_share(p);
>  
> -	return ret;
> +	return 0;
>  }