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  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]
Date:   Fri, 1 May 2020 15:44:03 +1000
From:   Greg Ungerer <>
To:     "Eric W. Biederman" <>,
        Linus Torvalds <>
Cc:     Russell King - ARM Linux admin <>,
        Jann Horn <>, Nicolas Pitre <>,
        Andrew Morton <>,
        Christoph Hellwig <>,
        Linux Kernel Mailing List <>,
        Linux-MM <>,
        linux-fsdevel <>,
        Alexander Viro <>,
        Oleg Nesterov <>,
        Linux ARM <>,
        Mark Salter <>,
        Aurelien Jacquiot <>,,
        Yoshinori Sato <>,
        Rich Felker <>,
        Linux-sh list <>
Subject: Re: [PATCH v2 0/5] Fix ELF / FDPIC ELF core dumping, and use mmap_sem
 properly in there

On 1/5/20 5:07 am, Eric W. Biederman wrote:
> Linus Torvalds <> writes:
>> On Thu, Apr 30, 2020 at 7:10 AM Greg Ungerer <> wrote:
>>>> Most of that file goes back to pre-git days. And most of the commits
>>>> since are not so much about binfmt_flat, as they are about cleanups or
>>>> changes elsewhere where binfmt_flat was just a victim.
>>> I'll have a look at this.
>> Thanks.
>>> Quick hack test shows moving setup_new_exec(bprm) to be just before
>>> install_exec_creds(bprm) works fine for the static binaries case.
>>> Doing the flush_old_exec(bprm) there too crashed out - I'll need to
>>> dig into that to see why.
>> Just moving setup_new_exec() would at least allow us to then join the
>> two together, and just say "setup_new_exec() does the credential
>> installation too".
> But it is only half a help if we allow failure points between
> flush_old_exec and install_exec_creds.
> Greg do things work acceptably if install_exec_creds is moved to right
> after setup_new_exec? (patch below)

Yes, confirmed. Worked fine with that patch applied.

> Looking at the code in load_flat_file after setup_new_exec it looks like
> the kinds of things that in binfmt_elf.c we do after install_exec_creds
> (aka vm_map).  So I think we want install_exec_creds sooner, instead
> of setup_new_exec later.
>> But if it's true that nobody really uses the odd flat library support
>> any more and there are no testers, maybe we should consider ripping it
>> out...
> I looked a little deeper and there is another reason to think about
> ripping out the flat library loader.  The code is recursive, and
> supports a maximum of 4 shared libraries in the entire system.
> load_flat_binary
> 	load_flat_file
>          	calc_reloc
>                  	load_flat_shared_libary
>                          	load_flat_file
>                                  	....
> I am mystified with what kind of system can survive with a grand total
> of 4 shared libaries.  I think my a.out slackware system that I ran on
> my i486 had more shared libraries.

The kind of embedded systems that were built with this stuff 20 years
ago didn't have lots of applications and libraries. I think we found
back then that most of your savings were from making libc shared.
Less significant gains from making other libraries shared. And there
was a bit of extra pain in setting them up with the shared library
code generation options (that had to be unique for each one).

The whole mechanism is a bit of hack, and there was a few other
limitations with the way it worked (I don't recall what they were
right now).

I am definitely in favor of removing it.


> Having read just a bit more it is definitely guaranteed (by the code)
> that the first time load_flat_file is called id 0 will be used (aka id 0
> is guaranteed to be the binary), and the ids 1, 2, 3 and 4 will only be
> used if a relocation includes that id to reference an external shared
> library.  That part of the code is drop dead simple.
> ---
> This is what I was thinking about applying.
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 831a2b25ba79..1a1d1fcb893f 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -541,6 +541,7 @@ static int load_flat_file(struct linux_binprm *bprm,
>   		/* OK, This is the point of no return */
>   		set_personality(PER_LINUX_32BIT);
>   		setup_new_exec(bprm);
> +		install_exec_creds(bprm);
>   	}
>   	/*
> @@ -963,8 +964,6 @@ static int load_flat_binary(struct linux_binprm *bprm)
>   		}
>   	}
> -	install_exec_creds(bprm);
> -
>   	set_binfmt(&flat_format);
>   #ifdef CONFIG_MMU

Powered by blists - more mailing lists