Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Kees Cook <keescook@chromium.org>
Cc:     linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
        Greg Ungerer <gerg@linux-m68k.org>,
        Rob Landley <rob@landley.net>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        linux-fsdevel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
References: <87h7wujhmz.fsf@x220.int.ebiederm.org>
        <87ftcei2si.fsf@x220.int.ebiederm.org>
        <202005051354.C7E2278688@keescook>
        <87368ddsc9.fsf@x220.int.ebiederm.org>
        <202005060829.A09C366D0@keescook>
Date:   Thu, 07 May 2020 14:51:47 -0500
In-Reply-To: <202005060829.A09C366D0@keescook> (Kees Cook's message of "Wed, 6
        May 2020 08:30:33 -0700")
Message-ID: <87d07fa5gs.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 6/7] exec: Move most of setup_new_exec into flush_old_exec
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Kees Cook <keescook@chromium.org> writes:

> On Wed, May 06, 2020 at 09:57:10AM -0500, Eric W. Biederman wrote:
>> Kees Cook <keescook@chromium.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.
>
> Right -- I looked through all of it (see my last email) and I think it's
> all okay, but I was curious if you'd looked too. :)

I had and I will finish looking in the other direction and see if there
is anything else I can see.

Thank you for asking and keeping me honest.  There are so many moving
parts to this code it is easy to overlook something by accident.

>> 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.
>
> Oh yay. :) Thanks for catching it!

Well that fix is going to be a lot more involved than I anticipated.
The more I looked the more bugs I find so I will revisit fixing that
after I complete this set of changes.  I thought it was going to be a
trivial localized fix, and unfortunately not.

Eric