linux-kernel - Re: [PATCH v3 10/16] exec: Remove do_execve

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: <20200708063525.GC4332@42.do-not-panic.com>
Date:   Wed, 8 Jul 2020 06:35:25 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     linux-kernel@...r.kernel.org, David Miller <davem@...emloft.net>,
        Greg Kroah-Hartman <greg@...ah.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>, bpf <bpf@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Gary Lin <GLin@...e.com>, Bruno Meneguele <bmeneg@...hat.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v3 10/16] exec: Remove do_execve_file

On Thu, Jul 02, 2020 at 11:41:34AM -0500, Eric W. Biederman wrote:
> Now that the last callser has been removed remove this code from exec.
> 
> For anyone thinking of resurrecing do_execve_file please note that
> the code was buggy in several fundamental ways.
> 
> - It did not ensure the file it was passed was read-only and that
>   deny_write_access had been called on it.  Which subtlely breaks
>   invaniants in exec.
> 
> - The caller of do_execve_file was expected to hold and put a
>   reference to the file, but an extra reference for use by exec was
>   not taken so that when exec put it's reference to the file an
>   underflow occured on the file reference count.

Maybe its my growing love with testing, but I'm going to have to partly
blame here that we added a new API without any respective testing.
Granted, I recall this this patch set could have used more wider review
and a bit more patience... but just mentioning this so we try to avoid
new api-without-testing with more reason in the future.

But more importantly, *how* could we have caught this? Or how can we
catch this sort of stuff better in the future?

> - The point of the interface was so that a pathname did not need to
>   exist.  Which breaks pathname based LSMs.

Perhaps so but this fails to do justice of the LSM consideration done
for the patch which added this during patch review [0], and I
particularly recall I called out LSM folks to bring their ray guns out at
this patch. It didn't get much attention.

Let me recap a few points I think your commit log should somehow
consider. You do as you please.

Users of shmem_kernel_file_setup() spawned out of the desire to                                                                                                      
*avoid* LSMs since it didn't make sense in their case as their inodes                                                                                                      
are never exposed to userspace. Such is the case for ipc/shm.c and                                                                                                         
security/keys/big_key.c. Refer to commit c7277090927a5 ("security: shmem:                                                                                                  
implement kernel private shmem inodes") and then commit e1832f2923ec9                                                                                                      
("ipc: use private shmem or hugetlbfs inodes for shm segments").

And the umh module approach was doing:

 a) mapping data already extracted by the kernel somehow from
    a file somehow, presumably from /lib/modules/ path somewhere, but
    again this is not visible to umc.c, as it just gets called with:

     fork_usermode_blob(void *data, size_t len, struct umh_info *info)

 b) Creating the respective tmpfs file with shmem_kernel_file_setup()
 c) Populating the file created and stuffing it with our data passed
 d) Calling do_execve_file() on it.

So, although I was hoping LSM folks would chime in for things I may have
missed during my patch review, my recollection from the patch thread was
that this becuase of a) it in theory could skip out on dealing with LSMs.

[0] https://lkml.kernel.org/r/20180509022526.hertzfpvy7apz6ny@ast-mbp               

  Luis