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