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

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

On Thu, Mar 12, 2020 at 10:43:00PM +0900, Tetsuo Handa wrote:
> Before thinking how to fix a bug that tomoyo_realpath_nofollow() from
> tomoyo_find_next_domain() likely fails with -ENOENT whenever
> fork_usermode_blob() is used because 449325b52b7a6208 did not take into
> account that TOMOYO security module needs to calculate symlink's pathname,
> is this a correct fix for a bug that file_inode(file)->i_writecount != 0
> and file->f_count < 0 ?

> -	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);
> +	}

*UGH*

	Something's certainly fishy with the refcounting there.
First of all, bprm->file is a counting reference (observe what
free_bprm() is doing).  So as it is, on success __do_execve_file()
consumes the reference passed to it in 'file', ditto for
do_execve_file().  However, it's inconsistent - failure of e.g.
bprm allocation leaves the reference unconsumed.  Your change
makes it consistent in that respect, but it means that in normal
case you are getting refcount higher by 1 than the mainline.
Does the mainline have an extra fput() *in* *normal* *case*?
I can easily believe in buggered cleanups on failure, but...
Has that code ever been tested?

	It _does_ look like that double-fput() is real, but
I'd like a confirmation before going further - umh is convoluted
enough for something subtle to be hidden there.  Alexei, what
the refcounting behaviour was supposed to be?  As in "this
function consumes the reference passed to it in this argument",
etc.