linux-kernel - Re: [PATCH v5 4/7] pidfd: Replace open-coded partial fd_install

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 6 Jul 2020 12:30:33 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     linux-kernel@...r.kernel.org, Sargun Dhillon <sargun@...gun.me>,
        Christian Brauner <christian@...uner.io>,
        Tycho Andersen <tycho@...ho.ws>,
        David Laight <David.Laight@...LAB.COM>,
        Christoph Hellwig <hch@....de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Aleksa Sarai <cyphar@...har.com>,
        Matt Denton <mpdenton@...gle.com>,
        Jann Horn <jannh@...gle.com>, Chris Palmer <palmer@...gle.com>,
        Robert Sesek <rsesek@...gle.com>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>, Shuah Khan <shuah@...nel.org>,
        netdev@...r.kernel.org, containers@...ts.linux-foundation.org,
        linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v5 4/7] pidfd: Replace open-coded partial
 fd_install_received()

On Mon, Jul 06, 2020 at 06:12:45PM +0200, Christian Brauner wrote:
> On Mon, Jul 06, 2020 at 08:34:06AM -0700, Kees Cook wrote:
> > Yup, this was a mistake in my refactoring of the pidfs changes.
> 
> I already did.

Er, what? (I had a typo in my quote: s/pidfs/pidfd/.) I was trying to
say that this was just a mistake in my refactoring of the pidfd usage of
the new helper.

> > 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 ;):

Agreed, "radical" was too strong.

> 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) {

But my point stands: I really dislike this; suddenly the caller needs to
manage this when it should be an entirely internal detail to the
function. It was only pidfd doing it wrong, and that was entirely my
fault in the conversion.

> 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.

I will rename the helper and add explicit documentation, but I really
don't think callers should have to deal with managing the helper's split
ref lifetime.

-- 
Kees Cook