Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 Aug 2022 13:09:06 +0200
From:   Gerald Schaefer <gerald.schaefer@linux.ibm.com>
To:     Brian Foster <bfoster@redhat.com>
Cc:     linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>
Subject: Re: [PATCH] s390: fix double free of GS and RI CBs on fork()
 failure
Message-ID: <20220817130906.0ce40da1@thinkpad>
In-Reply-To: <20220816155407.537372-1-bfoster@redhat.com>
References: <20220816155407.537372-1-bfoster@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 16 Aug 2022 11:54:07 -0400
Brian Foster <bfoster@redhat.com> wrote:

> The pointers for guarded storage and runtime instrumentation control
> blocks are stored in the thread_struct of the associated task. These
> pointers are initially copied on fork() via arch_dup_task_struct()
> and then cleared via copy_thread() before fork() returns. If fork()
> happens to fail after the initial task dup and before copy_thread(),
> the newly allocated task and associated thread_struct memory are
> freed via free_task() -> arch_release_task_struct(). This results in
> a double free of the guarded storage and runtime info structs
> because the fields in the failed task still refer to memory
> associated with the source task.
> 
> This problem can manifest as a BUG_ON() in set_freepointer() (with
> CONFIG_SLAB_FREELIST_HARDENED enabled) or KASAN splat (if enabled)
> when running trinity syscall fuzz tests on s390x. To avoid this
> problem, clear the associated pointer fields in
> arch_dup_task_struct() immediately after the new task is copied.
> Note that the RI flag is still cleared in copy_thread() because it
> resides in thread stack memory and that is where stack info is
> copied.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Hi all,
> 
> Note that I'm not subscribed to the list so please CC on reply. Further,
> I'm not terribly familiar with these associated features and so have not
> run any kind of functional testing here. My testing was purely around
> producing/preventing the double free issue. Any thoughts, reviews or
> further testing is appreciated. Thanks.
> 
> Brian
> 
>  arch/s390/kernel/process.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
> index 89949b9f3cf8..d5119e039d85 100644
> --- a/arch/s390/kernel/process.c
> +++ b/arch/s390/kernel/process.c
> @@ -91,6 +91,18 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>  
>  	memcpy(dst, src, arch_task_struct_size);
>  	dst->thread.fpu.regs = dst->thread.fpu.fprs;
> +
> +	/*
> +	 * Don't transfer over the runtime instrumentation or the guarded
> +	 * storage control block pointers. These fields are cleared here instead
> +	 * of in copy_thread() to avoid premature freeing of associated memory
> +	 * on fork() failure. Wait to clear the RI flag because ->stack still
> +	 * refers to the source thread.
> +	 */
> +	dst->thread.ri_cb = NULL;
> +	dst->thread.gs_cb = NULL;
> +	dst->thread.gs_bc_cb = NULL;
> +
>  	return 0;
>  }
>  
> @@ -150,13 +162,11 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>  	frame->childregs.flags = 0;
>  	if (new_stackp)
>  		frame->childregs.gprs[15] = new_stackp;
> -
> -	/* Don't copy runtime instrumentation info */
> -	p->thread.ri_cb = NULL;
> +	/*
> +	 * Clear the runtime instrumentation flag after the above childregs
> +	 * copy. The CB pointer was already cleared in arch_dup_task_struct().
> +	 */
>  	frame->childregs.psw.mask &= ~PSW_MASK_RI;
> -	/* Don't copy guarded storage control block */
> -	p->thread.gs_cb = NULL;
> -	p->thread.gs_bc_cb = NULL;
>  
>  	/* Set a new TLS ?  */
>  	if (clone_flags & CLONE_SETTLS) {

Thanks Brian, nice catch! Looks good to me. For completeness, we should
add stable / Fixes tags, like this:

Fixes: 8d9047f8b967c ("s390/runtime instrumentation: simplify task exit handling")
Fixes: 7b83c6297d2fc ("s390/guarded storage: simplify task exit handling")
Cc: <stable@vger.kernel.org> # 4.15

Not 100% sure about the Fixes tags, Heiko should also have a look when
he returns next week.

Reviewed-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>