linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sargun Dhillon <sargun@sargun.me>
To: Kees Cook <keescook@chromium.org>
Cc: Christian Brauner <christian.brauner@ubuntu.com>,
	containers@lists.linux-foundation.org,
	Giuseppe Scrivano <gscrivan@redhat.com>,
	Robert Sesek <rsesek@google.com>,
	Chris Palmer <palmer@google.com>, Jann Horn <jannh@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Daniel Wagner <daniel.wagner@bmw-carit.de>,
	linux-kernel@vger.kernel.org, Matt Denton <mpdenton@google.com>,
	John Fastabend <john.r.fastabend@intel.com>,
	linux-fsdevel@vger.kernel.org, Tejun Heo <tj@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	cgroups@vger.kernel.org, stable@vger.kernel.org,
	"David S . Miller" <davem@davemloft.net>
Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes
Date: Wed, 10 Jun 2020 08:12:38 +0000	[thread overview]
Message-ID: <20200610081237.GA23425@ircssh-2.c.rugged-nimbus-611.internal> (raw)
In-Reply-To: <202006092227.D2D0E1F8F@keescook>

On Tue, Jun 09, 2020 at 10:27:54PM -0700, Kees Cook wrote:
> On Tue, Jun 09, 2020 at 11:27:30PM +0200, Christian Brauner wrote:
> > On June 9, 2020 10:55:42 PM GMT+02:00, Kees Cook <keescook@chromium.org> wrote:
> > >LOL. And while we were debating this, hch just went and cleaned stuff up:
> > >
> > >2618d530dd8b ("net/scm: cleanup scm_detach_fds")
> > >
> > >So, um, yeah, now my proposal is actually even closer to what we already
> > >have there. We just add the replace_fd() logic to __scm_install_fd() and
> > >we're done with it.
> > 
> > Cool, you have a link? :)
> 
> How about this:
> 
Thank you.
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=devel/seccomp/addfd/v3.1&id=bb94586b9e7cc88e915536c2e9fb991a97b62416
> 
> -- 
> Kees Cook

+		if (ufd) {
+			error = put_user(new_fd, ufd);
+			if (error) {
+				put_unused_fd(new_fd);
+				return error;
+			}
+ 		}
I'm fairly sure this introduces a bug[1] if the user does:

struct msghdr msg = {};
struct cmsghdr *cmsg;
struct iovec io = {
	.iov_base = &c,
	.iov_len = 1,
};

msg.msg_iov = &io;
msg.msg_iovlen = 1;
msg.msg_control = NULL;
msg.msg_controllen = sizeof(buf);

recvmsg(sock, &msg, 0);

They will have the FD installed, no error message, but FD number wont be written 
to memory AFAICT. If two FDs are passed, you will get an efault. They will both
be installed, but memory wont be written to. Maybe instead of 0, make it a
poison pointer, or -1 instead?

-----
As an aside, all of this junk should be dropped:
+	ret = get_user(size, &uaddfd->size);
+	if (ret)
+		return ret;
+
+	ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
+	if (ret)
+		return ret;

and the size member of the seccomp_notif_addfd struct. I brought this up 
off-list with Tycho that ioctls have the size of the struct embedded in them. We 
should just use that. The ioctl definition is based on this[2]:
#define _IOC(dir,type,nr,size) \
	(((dir)  << _IOC_DIRSHIFT) | \
	 ((type) << _IOC_TYPESHIFT) | \
	 ((nr)   << _IOC_NRSHIFT) | \
	 ((size) << _IOC_SIZESHIFT))


We should just use copy_from_user for now. In the future, we can either 
introduce new ioctl names for new structs, or extract the size dynamically from 
the ioctl (and mask it out on the switch statement in seccomp_notify_ioctl.

----
+#define SECCOMP_IOCTL_NOTIF_ADDFD	SECCOMP_IOR(3,	\
+						struct seccomp_notif_addfd)

Lastly, what I believe to be a small mistake, it should be SECCOMP_IOW, based on 
the documentation in ioctl.h -- "_IOW means userland is writing and kernel is 
reading."


[1]: https://lore.kernel.org/lkml/20200604052040.GA16501@ircssh-2.c.rugged-nimbus-611.internal/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/ioctl.h?id=v5.7#n69

  reply	other threads:[~2020-06-10  8:12 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-03  1:10 [PATCH v3 0/4] Add seccomp notifier ioctl that enables adding fds Sargun Dhillon
2020-06-03  1:10 ` [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Sargun Dhillon
2020-06-04  1:24   ` Christian Brauner
2020-06-04  2:22     ` Kees Cook
2020-06-04  5:20       ` Sargun Dhillon
2020-06-04 12:52       ` Christian Brauner
2020-06-04 13:28         ` David Laight
2020-06-05  7:54         ` Sargun Dhillon
2020-06-09 19:43           ` Kees Cook
2020-06-09 20:03             ` Christian Brauner
2020-06-09 20:55               ` Kees Cook
2020-06-09 21:27                 ` Christian Brauner
2020-06-10  5:27                   ` Kees Cook
2020-06-10  8:12                     ` Sargun Dhillon [this message]
2020-06-10  8:48                       ` David Laight
2020-06-11  3:02                         ` Kees Cook
2020-06-11  7:51                           ` David Laight
2020-06-10 17:10                       ` Kees Cook
2020-06-11  2:59                       ` Kees Cook
2020-06-11  4:41                         ` Sargun Dhillon
2020-06-11  9:19                         ` Christian Brauner
2020-06-11 10:39                           ` Sargun Dhillon
2020-06-11 23:23                             ` Kees Cook
2020-06-11 10:01                         ` Christian Brauner
2020-06-11 11:06                           ` Sargun Dhillon
2020-06-11 14:42                             ` Christian Brauner
2020-06-11 14:56                             ` David Laight
2020-06-11 23:49                               ` Kees Cook
2020-06-12  6:58                                 ` Kees Cook
2020-06-12  8:36                                 ` David Laight
2020-06-12 10:46                                   ` Sargun Dhillon
2020-06-12 15:13                                     ` Kees Cook
2020-06-12 15:55                                       ` David Laight
2020-06-12 18:28                                       ` Christian Brauner
2020-06-12 18:38                                         ` Kees Cook
2020-06-12 18:42                                           ` Christian Brauner
2020-06-15  8:27                                         ` David Laight
2020-06-10  9:30                   ` Christian Brauner
2020-06-04  3:39     ` Sargun Dhillon
2020-06-03  1:10 ` [PATCH v3 2/4] pid: Use file_receive helper to copy FDs Sargun Dhillon
2020-06-03  1:10 ` [PATCH v3 3/4] seccomp: Introduce addfd ioctl to seccomp user notifier Sargun Dhillon
2020-06-03  1:10 ` [PATCH v3 4/4] selftests/seccomp: Test SECCOMP_IOCTL_NOTIF_ADDFD Sargun Dhillon
2020-06-03 21:25 ` [PATCH v3 0/4] Add seccomp notifier ioctl that enables adding fds Robert Sesek
2020-06-03 23:42 ` Kees Cook
2020-06-03 23:56   ` Sargun Dhillon
2020-06-04  2:44     ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200610081237.GA23425@ircssh-2.c.rugged-nimbus-611.internal \
    --to=sargun@sargun.me \
    --cc=cgroups@vger.kernel.org \
    --cc=christian.brauner@ubuntu.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=daniel.wagner@bmw-carit.de \
    --cc=davem@davemloft.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=gscrivan@redhat.com \
    --cc=jannh@google.com \
    --cc=john.r.fastabend@intel.com \
    --cc=keescook@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpdenton@google.com \
    --cc=palmer@google.com \
    --cc=rsesek@google.com \
    --cc=stable@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).