Re: [PATCH (repost)] umh: fix refcount underflow in fork_usermode_blob().

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Mar 27, 2020 at 09:51:34AM +0900, Tetsuo Handa wrote:

> diff --git a/fs/exec.c b/fs/exec.c
> index db17be51b112..ded3fa368dc7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1761,11 +1761,17 @@ static int __do_execve_file(int fd, struct filename *filename,
>  	check_unsafe_exec(bprm);
>  	current->in_execve = 1;
>  
> -	if (!file)
> +	if (!file) {
>  		file = do_open_execat(fd, filename, flags);
> -	retval = PTR_ERR(file);
> -	if (IS_ERR(file))
> -		goto out_unmark;
> +		retval = PTR_ERR(file);
> +		if (IS_ERR(file))
> +			goto out_unmark;
> +	} else {
> +		retval = deny_write_access(file);
> +		if (retval)
> +			goto out_unmark;
> +		get_file(file);
> +	}

I still don't like it.  The bug is real, but... *yeccchhhh*

First of all, this assignment to "file" is misguiding -
assignment to bprm->file would've been a lot easier to
follow.  Furthermore, the damn thing already has much
too confusing cleanup logics.

Why is
out:
        if (bprm->mm) {
                acct_arg_size(bprm, 0);
                mmput(bprm->mm);
        }
done on failure exit in this function and not in free_bprm(),
while dropping bprm->file is in free_bprm()?

Note that flush_old_exec() will zero bprm->mm (after it transfers
the damn thing into current->mm), so we are fine here.  And getting
rid of that thing in __do_execve_file() simplifies the logics
in there, especially if you take everything from this
        if (!file)
up to
        retval = exec_binprm(bprm);
into a new function.  All those goto out_unmark/goto out turn
into plain returns.  And in __do_execve_file() we are left with
        ....
        current->in_execve = 1;
	retval = this_new_helper(whatever it needs passed to it);
        current->fs->in_exec = 0;
        current->in_execve = 0;
	if (!retval) {
		/* execve succeeded */
		rseq_execve(current);
		acct_update_integrals(current);
		task_numa_free(current, false);
	}
out_free:
        free_bprm(bprm);
        kfree(pathbuf);
out_files:
        if (displaced)
                put_files_struct(displaced);
out_ret:
        if (filename)
                putname(filename);
        return retval;
which is a lot easier to follow.  Especially if we lift the logics
for freeing pathbuf into free_bprm() as well (will need a flag there,
for "does ->filename need to be freed?").  And I really wonder if
sched_exec() is called in the right place - what's special about the
point after opening the binary to be and setting bprm->file to what
we got?