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

From: Al Viro <viro@zeniv.linux.org.uk>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH (repost)] umh: fix refcount underflow in fork_usermode_blob().
Date: Sun, 29 Mar 2020 04:17:02 +0100	[thread overview]
Message-ID: <20200329031702.GB23230@ZenIV.linux.org.uk> (raw)
In-Reply-To: <9b846b1f-a231-4f09-8c37-6bfb0d1e7b05@i-love.sakura.ne.jp>

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?