linux-kernel - Re: [PATCH] kthread: fix boot hang (regression) on MIPS/OpenRISC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170529125533.GB2940@lianli.shorne-pla.net>
Date:   Mon, 29 May 2017 21:55:33 +0900
From:   Stafford Horne <shorne@...il.com>
To:     Vegard Nossum <vegard.nossum@...cle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mips@...ux-mips.org,
        Jonas Bonn <jonas@...thpole.se>,
        Stefan Kristiansson <stefan.kristiansson@...nalahti.fi>,
        openrisc@...ts.librecores.org, Oleg Nesterov <oleg@...hat.com>,
        Jamie Iles <jamie.iles@...cle.com>
Subject: Re: [PATCH] kthread: fix boot hang (regression) on MIPS/OpenRISC

On Mon, May 29, 2017 at 09:22:07AM +0200, Vegard Nossum wrote:
> This fixes a regression in commit 4d6501dce079 where I didn't notice
> that MIPS and OpenRISC were reinitialising p->{set,clear}_child_tid to
> NULL after our initialisation in copy_process().
> 
> We can simply get rid of the arch-specific initialisation here since it
> is now always done in copy_process() before hitting copy_thread{,_tls}().
> 
> Review notes:
> 
>  - As far as I can tell, copy_process() is the only user of
>    copy_thread_tls(), which is the only caller of copy_thread() for
>    architectures that don't implement copy_thread_tls().
> 
>  - After this patch, there is no arch-specific code touching
>    p->set_child_tid or p->clear_child_tid whatsoever.
> 
>  - It may look like MIPS/OpenRISC wanted to always have these fields be
>    NULL, but that's not true, as copy_process() would unconditionally
>    set them again _after_ calling copy_thread_tls() before commit
>    4d6501dce079.
> 
> Fixes: 4d6501dce079c1eb6bf0b1d8f528a5e81770109e ("kthread: Fix use-after-free if kthread fork fails")
> Reported-by: Guenter Roeck <linux@...ck-us.net>
> Tested-by: Guenter Roeck <linux@...ck-us.net> # MIPS only
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: linux-mips@...ux-mips.org
> Cc: Jonas Bonn <jonas@...thpole.se>
> Cc: Stefan Kristiansson <stefan.kristiansson@...nalahti.fi>
> Cc: Stafford Horne <shorne@...il.com>
> Cc: openrisc@...ts.librecores.org
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: Jamie Iles <jamie.iles@...cle.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Vegard Nossum <vegard.nossum@...cle.com>
> ---
> Not sure who this should go through, the last patch went through tglx/the
> core-urgent-for-linus tree, but it does touch arch code + fix a mainline
> boot hang regression on at least MIPS (Guenter said OpenRISC didn't seem
> affected in his boot tests, but the code looks wrong in any case). Maybe
> we could get acks/reviews by MIPS and OpenRISC maintainers?

This looks ok with me, I am pretty sure a lot of the OpenRISC initial port
was based on mips so this could have been copied from the beginning.

Acked-by: Stafford Horne <shorne@...il.com>

> ---
>  arch/mips/kernel/process.c     | 1 -
>  arch/openrisc/kernel/process.c | 2 --
>  2 files changed, 3 deletions(-)
> 
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index 918d4c73e951..5351e1f3950d 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -120,7 +120,6 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
>  	struct thread_info *ti = task_thread_info(p);
>  	struct pt_regs *childregs, *regs = current_pt_regs();
>  	unsigned long childksp;
> -	p->set_child_tid = p->clear_child_tid = NULL;
>  
>  	childksp = (unsigned long)task_stack_page(p) + THREAD_SIZE - 32;
>  
> diff --git a/arch/openrisc/kernel/process.c b/arch/openrisc/kernel/process.c
> index f8da545854f9..106859ae27ff 100644
> --- a/arch/openrisc/kernel/process.c
> +++ b/arch/openrisc/kernel/process.c
> @@ -167,8 +167,6 @@ copy_thread(unsigned long clone_flags, unsigned long usp,
>  
>  	top_of_kernel_stack = sp;
>  
> -	p->set_child_tid = p->clear_child_tid = NULL;
> -
>  	/* Locate userspace context on stack... */
>  	sp -= STACK_FRAME_OVERHEAD;	/* redzone */
>  	sp -= sizeof(struct pt_regs);
> -- 
> 2.12.0.rc0
>