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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <202005060829.A09C366D0@keescook>
Date:   Wed, 6 May 2020 08:30:33 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
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

On Wed, May 06, 2020 at 09:57:10AM -0500, Eric W. Biederman wrote:
> 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.

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. :)

> 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!

-- 
Kees Cook