linux-kernel - Re: [PATCH 6/7] exec: Move most of setup_new_exec into flush_old

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87368ddsc9.fsf@x220.int.ebiederm.org>
Date:   Wed, 06 May 2020 09:57:10 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>, Jann Horn <jannh@...gle.com>,
        Greg Ungerer <gerg@...ux-m68k.org>,
        Rob Landley <rob@...dley.net>,
        Bernd Edlinger <bernd.edlinger@...mail.de>,
        linux-fsdevel@...r.kernel.org, Al Viro <viro@...IV.linux.org.uk>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 6/7] exec: Move most of setup_new_exec into flush_old_exec

Kees Cook <keescook@...omium.org> writes:

> On Tue, May 05, 2020 at 02:45:33PM -0500, Eric W. Biederman wrote:
>> 
>> The current idiom for the callers is:
>> 
>> flush_old_exec(bprm);
>> set_personality(...);
>> setup_new_exec(bprm);
>> 
>> In 2010 Linus split flush_old_exec into flush_old_exec and
>> setup_new_exec.  With the intention that setup_new_exec be what is
>> called after the processes new personality is set.
>> 
>> Move the code that doesn't depend upon the personality from
>> setup_new_exec into flush_old_exec.  This is to facilitate future
>> changes by having as much code together in one function as possible.
>
> Er, I *think* this is okay, but I have some questions below which
> maybe you already investigated (and should perhaps get called out in
> the changelog).

I will see if I can expand more on the review that I have done.

I saw this as moving thre lines and the personality setting later in the
code, rather than moving a bunch of lines up

AKA these lines:
>> +	arch_pick_mmap_layout(me->mm, &bprm->rlim_stack);
>> +
>> +	arch_setup_new_exec();
>> +
>> +	/* Set the new mm task size. We have to do that late because it may
>> +	 * depend on TIF_32BIT which is only updated in flush_thread() on
>> +	 * some architectures like powerpc
>> +	 */
>> +	me->mm->task_size = TASK_SIZE;

I verified carefully that only those three lines can depend upon the
personality changes.

Your concern if anything depends on those moved lines I haven't looked
at so closely so I will go back through and do that.  I don't actually
expect anything depends upon those three lines because they should only
be changing architecture specific state.  But that is general handwaving
not actually careful review which tends to turn up suprises in exec.

Speaking of while I was looking through the lsm hooks again I just
realized that 613cc2b6f272 ("fs: exec: apply CLOEXEC before changing
dumpable task flags") only fixed half the problem.  So I am going to
take a quick detour fix that then come back to this.  As that directly
affects this code motion.

Eric