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@chromium.org
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