diff mbox series

[v2,10/15] exec: Remove do_execve_file

Message ID 87lfk54p0m.fsf_-_@x220.int.ebiederm.org
State Not Applicable
Delegated to: David Miller
Headers show
Series Make the user mode driver code a better citizen | expand

Commit Message

Eric W. Biederman June 29, 2020, 8:04 p.m. UTC 
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.

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

Tetsuo Handa originally reported these issues[1].  While it was clear
that deny_write_access was missing the fundamental incompatibility
with the passed in O_RDWR filehandle was not immediately recognized.

All of these issues were fixed by modifying the usermode driver code
to have a path, so it did not need this hack.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[1] https://lore.kernel.org/linux-fsdevel/2a8775b4-1dd5-9d5c-aa42-9872445e0942@i-love.sakura.ne.jp/
Link: https://lkml.kernel.org/r/871rm2f0hi.fsf_-_@x220.int.ebiederm.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c               | 38 +++++++++-----------------------------
 include/linux/binfmts.h |  1 -
 2 files changed, 9 insertions(+), 30 deletions(-)

Comments

Christoph Hellwig June 30, 2020, 5:43 a.m. UTC | #1 
FYI, this clashes badly with my exec rework.  I'd suggest you
drop everything touching exec here for now, and I can then
add the final file based exec removal to the end of my series.
Eric W. Biederman June 30, 2020, 12:14 p.m. UTC | #2 
Christoph Hellwig <hch@infradead.org> writes:

> FYI, this clashes badly with my exec rework.  I'd suggest you
> drop everything touching exec here for now, and I can then
> add the final file based exec removal to the end of my series.

I have looked and I haven't even seen any exec work.  Where can it be
found?

I have working and cleaning up exec for what 3 cycles now.  There is
still quite a ways to go before it becomes possible to fix some of the
deep problems in exec.  Removing all of these broken exec special cases
is quite frankly the entire point of this patchset.

Sight unseen I suggest you send me your exec work and I can merge it
into my branch if we are going to conflict badly.

Eric
Christoph Hellwig June 30, 2020, 1:38 p.m. UTC | #3 
On Tue, Jun 30, 2020 at 07:14:23AM -0500, Eric W. Biederman wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > FYI, this clashes badly with my exec rework.  I'd suggest you
> > drop everything touching exec here for now, and I can then
> > add the final file based exec removal to the end of my series.
> 
> I have looked and I haven't even seen any exec work.  Where can it be
> found?
> 
> I have working and cleaning up exec for what 3 cycles now.  There is
> still quite a ways to go before it becomes possible to fix some of the
> deep problems in exec.  Removing all of these broken exec special cases
> is quite frankly the entire point of this patchset.
> 
> Sight unseen I suggest you send me your exec work and I can merge it
> into my branch if we are going to conflict badly.

https://lore.kernel.org/linux-fsdevel/20200627072704.2447163-1-hch@lst.de/T/#t
Eric W. Biederman June 30, 2020, 2:28 p.m. UTC | #4 
Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jun 30, 2020 at 07:14:23AM -0500, Eric W. Biederman wrote:
>> Christoph Hellwig <hch@infradead.org> writes:
>> 
>> > FYI, this clashes badly with my exec rework.  I'd suggest you
>> > drop everything touching exec here for now, and I can then
>> > add the final file based exec removal to the end of my series.
>> 
>> I have looked and I haven't even seen any exec work.  Where can it be
>> found?
>> 
>> I have working and cleaning up exec for what 3 cycles now.  There is
>> still quite a ways to go before it becomes possible to fix some of the
>> deep problems in exec.  Removing all of these broken exec special cases
>> is quite frankly the entire point of this patchset.
>> 
>> Sight unseen I suggest you send me your exec work and I can merge it
>> into my branch if we are going to conflict badly.
>
> https://lore.kernel.org/linux-fsdevel/20200627072704.2447163-1-hch@lst.de/T/#t


Looking at your final patch I do not like the construct.

static int __do_execveat(int fd, struct filename *filename,
 		const char __user *const __user *argv,
 		const char __user *const __user *envp,
		const char *const *kernel_argv,
		const char *const *kernel_envp,
 		int flags, struct file *file);


It results in a function that is full of:
	if (kernel_argv) {
        	// For kernel_exeveat 
		...
	} else {
        	// For ordinary exeveat
        	
        }

Which while understandable.  I do not think results in good long term
maintainble code.

The current file paramter that I am getting rid of in my patchset is
a stark example of that.  Because of all of the if's no one realized
that the code had it's file reference counting wrong (amoung other
bugs).

I think this is important to address as exec has already passed
the point where people can fix all of the bugs in exec because
the code is so hairy.

I think to be maintainable and clear the code exec code is going to
need to look something like:

static int bprm_execveat(int fd, struct filename *filename,
			struct bprm *bprm, int flags);

int kernel_execve(const char *filename,
		  const char *const *argv, const char *const *envp, int flags)
{
	bprm = kzalloc(sizeof(*pbrm), GFP_KERNEL);
        bprm->argc = count_kernel_strings(argv);
        bprm->envc = count_kernel_strings(envp);
        prepare_arg_pages(bprm);
        copy_strings_kernel(bprm->envc, envp, bprm);
        copy_strings_kernel(bprm->argc, argc, bprm);
	ret = bprm_execveat(AT_FDCWD, filename, bprm);
        free_bprm(bprm);
        return ret;
}

int do_exeveat(int fd, const char *filename,
		const char __user *const __user *argv,
                const char __user *const __user *envp, int flags)
{
	bprm = kzalloc(sizeof(*pbrm), GFP_KERNEL);
        bprm->argc = count_strings(argv);
        bprm->envc = count_strings(envp);
        prepare_arg_pages(bprm);
        copy_strings(bprm->envc, envp, bprm);
        copy_strings(bprm->argc, argc, bprm);
	ret = bprm_execveat(fd, filename, bprm);
        free_bprm(bprm);
        return ret;
}

More work is required obviously to make the code above really work but
when the dust clears a structure like that doesn't have funny edge cases
that can hide bugs and make it tricky to change the code.

Eric
Alexei Starovoitov June 30, 2020, 4:55 p.m. UTC | #5 
On Tue, Jun 30, 2020 at 09:28:10AM -0500, Eric W. Biederman wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Tue, Jun 30, 2020 at 07:14:23AM -0500, Eric W. Biederman wrote:
> >> Christoph Hellwig <hch@infradead.org> writes:
> >> 
> >> > FYI, this clashes badly with my exec rework.  I'd suggest you
> >> > drop everything touching exec here for now, and I can then
> >> > add the final file based exec removal to the end of my series.
> >> 
> >> I have looked and I haven't even seen any exec work.  Where can it be
> >> found?
> >> 
> >> I have working and cleaning up exec for what 3 cycles now.  There is
> >> still quite a ways to go before it becomes possible to fix some of the
> >> deep problems in exec.  Removing all of these broken exec special cases
> >> is quite frankly the entire point of this patchset.
> >> 
> >> Sight unseen I suggest you send me your exec work and I can merge it
> >> into my branch if we are going to conflict badly.
> >
> > https://lore.kernel.org/linux-fsdevel/20200627072704.2447163-1-hch@lst.de/T/#t
> 
> 
> Looking at your final patch I do not like the construct.
> 
> static int __do_execveat(int fd, struct filename *filename,
>  		const char __user *const __user *argv,
>  		const char __user *const __user *envp,
> 		const char *const *kernel_argv,
> 		const char *const *kernel_envp,
>  		int flags, struct file *file);
> 
> 
> It results in a function that is full of:
> 	if (kernel_argv) {
>         	// For kernel_exeveat 
> 		...
> 	} else {
>         	// For ordinary exeveat
>         	
>         }
> 
> Which while understandable.  I do not think results in good long term
> maintainble code.
> 
> The current file paramter that I am getting rid of in my patchset is
> a stark example of that.  Because of all of the if's no one realized
> that the code had it's file reference counting wrong (amoung other
> bugs).
> 
> I think this is important to address as exec has already passed
> the point where people can fix all of the bugs in exec because
> the code is so hairy.
> 
> I think to be maintainable and clear the code exec code is going to
> need to look something like:
> 
> static int bprm_execveat(int fd, struct filename *filename,
> 			struct bprm *bprm, int flags);
> 
> int kernel_execve(const char *filename,
> 		  const char *const *argv, const char *const *envp, int flags)
> {
> 	bprm = kzalloc(sizeof(*pbrm), GFP_KERNEL);
>         bprm->argc = count_kernel_strings(argv);
>         bprm->envc = count_kernel_strings(envp);
>         prepare_arg_pages(bprm);
>         copy_strings_kernel(bprm->envc, envp, bprm);
>         copy_strings_kernel(bprm->argc, argc, bprm);
> 	ret = bprm_execveat(AT_FDCWD, filename, bprm);
>         free_bprm(bprm);
>         return ret;
> }
> 
> int do_exeveat(int fd, const char *filename,
> 		const char __user *const __user *argv,
>                 const char __user *const __user *envp, int flags)
> {
> 	bprm = kzalloc(sizeof(*pbrm), GFP_KERNEL);
>         bprm->argc = count_strings(argv);
>         bprm->envc = count_strings(envp);
>         prepare_arg_pages(bprm);
>         copy_strings(bprm->envc, envp, bprm);
>         copy_strings(bprm->argc, argc, bprm);
> 	ret = bprm_execveat(fd, filename, bprm);
>         free_bprm(bprm);
>         return ret;
> }
> 
> More work is required obviously to make the code above really work but
> when the dust clears a structure like that doesn't have funny edge cases
> that can hide bugs and make it tricky to change the code.

+1 to the approach.
I think Christoph's work need to be on top of Eric's.
diff mbox series

Patch

diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..23dfbb820626 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1818,13 +1818,14 @@  static int exec_binprm(struct linux_binprm *bprm)
 /*
  * sys_execve() executes a new program.
  */
-static int __do_execve_file(int fd, struct filename *filename,
-			    struct user_arg_ptr argv,
-			    struct user_arg_ptr envp,
-			    int flags, struct file *file)
+static int do_execveat_common(int fd, struct filename *filename,
+			      struct user_arg_ptr argv,
+			      struct user_arg_ptr envp,
+			      int flags)
 {
 	char *pathbuf = NULL;
 	struct linux_binprm *bprm;
+	struct file *file;
 	struct files_struct *displaced;
 	int retval;
 
@@ -1863,8 +1864,7 @@  static int __do_execve_file(int fd, struct filename *filename,
 	check_unsafe_exec(bprm);
 	current->in_execve = 1;
 
-	if (!file)
-		file = do_open_execat(fd, filename, flags);
+	file = do_open_execat(fd, filename, flags);
 	retval = PTR_ERR(file);
 	if (IS_ERR(file))
 		goto out_unmark;
@@ -1872,9 +1872,7 @@  static int __do_execve_file(int fd, struct filename *filename,
 	sched_exec();
 
 	bprm->file = file;
-	if (!filename) {
-		bprm->filename = "none";
-	} else if (fd == AT_FDCWD || filename->name[0] == '/') {
+	if (fd == AT_FDCWD || filename->name[0] == '/') {
 		bprm->filename = filename->name;
 	} else {
 		if (filename->name[0] == '\0')
@@ -1935,8 +1933,7 @@  static int __do_execve_file(int fd, struct filename *filename,
 	task_numa_free(current, false);
 	free_bprm(bprm);
 	kfree(pathbuf);
-	if (filename)
-		putname(filename);
+	putname(filename);
 	if (displaced)
 		put_files_struct(displaced);
 	return retval;
@@ -1967,27 +1964,10 @@  static int __do_execve_file(int fd, struct filename *filename,
 	if (displaced)
 		reset_files_struct(displaced);
 out_ret:
-	if (filename)
-		putname(filename);
+	putname(filename);
 	return retval;
 }
 
-static int do_execveat_common(int fd, struct filename *filename,
-			      struct user_arg_ptr argv,
-			      struct user_arg_ptr envp,
-			      int flags)
-{
-	return __do_execve_file(fd, filename, argv, envp, flags, NULL);
-}
-
-int do_execve_file(struct file *file, void *__argv, void *__envp)
-{
-	struct user_arg_ptr argv = { .ptr.native = __argv };
-	struct user_arg_ptr envp = { .ptr.native = __envp };
-
-	return __do_execve_file(AT_FDCWD, NULL, argv, envp, 0, file);
-}
-
 int do_execve(struct filename *filename,
 	const char __user *const __user *__argv,
 	const char __user *const __user *__envp)
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 4a20b7517dd0..7c27d7b57871 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -141,6 +141,5 @@  extern int do_execveat(int, struct filename *,
 		       const char __user * const __user *,
 		       const char __user * const __user *,
 		       int);
-int do_execve_file(struct file *file, void *__argv, void *__envp);
 
 #endif /* _LINUX_BINFMTS_H */