Re: [PATCH v5 4/7] pidfd: Replace open-coded partial fd_install_received()

[Christian Brauner]

On Mon, Jul 06, 2020 at 08:34:06AM -0700, Kees Cook wrote:
> On Mon, Jul 06, 2020 at 03:07:13PM +0200, Christian Brauner wrote:
> > On Wed, Jun 17, 2020 at 03:03:24PM -0700, Kees Cook wrote:
> > > The sock counting (sock_update_netprioidx() and sock_update_classid()) was
> > > missing from pidfd's implementation of received fd installation. Replace
> > > the open-coded version with a call to the new fd_install_received()
> > > helper.
> > > 
> > > Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > > ---
> > >  kernel/pid.c | 11 +----------
> > >  1 file changed, 1 insertion(+), 10 deletions(-)
> > > 
> > > diff --git a/kernel/pid.c b/kernel/pid.c
> > > index f1496b757162..24924ec5df0e 100644
> > > --- a/kernel/pid.c
> > > +++ b/kernel/pid.c
> > > @@ -635,18 +635,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
> > >  	if (IS_ERR(file))
> > >  		return PTR_ERR(file);
> > >  
> > > -	ret = security_file_receive(file);
> > > -	if (ret) {
> > > -		fput(file);
> > > -		return ret;
> > > -	}
> > > -
> > > -	ret = get_unused_fd_flags(O_CLOEXEC);
> > > +	ret = fd_install_received(file, O_CLOEXEC);
> > >  	if (ret < 0)
> > >  		fput(file);
> > > -	else
> > > -		fd_install(ret, file);
> > 
> > So someone just sent a fix for pidfd_getfd() that was based on the
> > changes done here.
> 
> Hi! Ah yes, that didn't get CCed to me. I'll go reply.
> 
> > I've been on vacation so didn't have a change to review this series and
> > I see it's already in linux-next. This introduces a memory leak and
> > actually proves a point I tried to stress when adding this helper:
> > fd_install_received() in contrast to fd_install() does _not_ consume a
> > reference because it takes one before it calls into fd_install(). That
> > means, you need an unconditional fput() here both in the failure and
> > error path.
> 
> Yup, this was a mistake in my refactoring of the pidfs changes.

I already did.

> 
> > I strongly suggest though that we simply align the behavior between
> > fd_install() and fd_install_received() and have the latter simply
> > consume a reference when it succeeds! Imho, this bug proves that I was
> > right to insist on this before. ;)
> 
> I still don't agree: it radically complicates the SCM_RIGHTS and seccomp

I'm sorry, I don't buy it yet, though I might've missed something in the
discussions: :)
After applying the patches in your series this literally is just (which
is hardly radical ;):

diff --git a/fs/file.c b/fs/file.c
index 9568bcfd1f44..26930b2ea39d 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -974,7 +974,7 @@ int __fd_install_received(int fd, struct file *file, int __user *ufd,
        }

        if (fd < 0)
-               fd_install(new_fd, get_file(file));
+               fd_install(new_fd, file);
        else {
                new_fd = fd;
                error = replace_fd(new_fd, file, o_flags);
diff --git a/net/compat.c b/net/compat.c
index 71494337cca7..605a5a67200c 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -298,9 +298,11 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
        int err = 0, i;

        for (i = 0; i < fdmax; i++) {
-               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
-               if (err < 0)
+               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
+               if (err < 0) {
+                       fput(scm->fp->fp[i]);
                        break;
+               }
        }

        if (i > 0) {
diff --git a/net/core/scm.c b/net/core/scm.c
index b9a0442ebd26..0d06446ae598 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -306,9 +306,11 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
        }

        for (i = 0; i < fdmax; i++) {
-               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
-               if (err < 0)
+               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
+               if (err < 0) {
+                       fput(scm->fp->fp[i]);
                        break;
+               }
        }

        if (i > 0) {

> cases. The primary difference is that fd_install() cannot fail, and it
> was optimized for this situation. The other file-related helpers that
> can fail do not consume the reference, so this is in keeping with those
> as well.

That's not a real problem. Any function that can fail and which consumes
a reference on success is assumed to not mutate the reference if it
fails anywhere. So I don't see that as an issue.

The problem here is that the current patch invites bugs and has already
produced one because fd_install() and fd_install_*() have the same
naming scheme but different behavior when dealing with references.
That's just not a good idea.

Christian