On Thu, Jun 04, 2020 at 02:52:26PM +0200, Christian Brauner wrote:

On Wed, Jun 03, 2020 at 07:22:57PM -0700, Kees Cook wrote:

...

On Thu, Jun 04, 2020 at 03:24:52AM +0200, Christian Brauner wrote:

...

On Tue, Jun 02, 2020 at 06:10:41PM -0700, Sargun Dhillon wrote:

...

Previously there were two chunks of code where the logic to receive file descriptors was duplicated in net. The compat version of copying file descriptors via SCM_RIGHTS did not have logic to update cgroups. Logic to change the cgroup data was added in: commit 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly") commit d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")

This was not copied to the compat path. This commit fixes that, and thus should be cherry-picked into stable.

This introduces a helper (file_receive) which encapsulates the logic for handling calling security hooks as well as manipulating cgroup information. This helper can then be used other places in the kernel where file descriptors are copied between processes

I tested cgroup classid setting on both the compat (x32) path, and the native path to ensure that when moving the file descriptor the classid is set.

Signed-off-by: Sargun Dhillon sargun@sargun.me Suggested-by: Kees Cook keescook@chromium.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: Christian Brauner christian.brauner@ubuntu.com Cc: Daniel Wagner daniel.wagner@bmw-carit.de Cc: David S. Miller davem@davemloft.net Cc: Jann Horn jannh@google.com, Cc: John Fastabend john.r.fastabend@intel.com Cc: Tejun Heo tj@kernel.org Cc: Tycho Andersen tycho@tycho.ws Cc: stable@vger.kernel.org Cc: cgroups@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org

---

fs/file.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/file.h | 1 + net/compat.c | 10 +++++----- net/core/scm.c | 14 ++++---------- 4 files changed, 45 insertions(+), 15 deletions(-)

This is all just a remote version of fd_install(), yet it deviates from fd_install()'s semantics and naming. That's not great imho. What about naming this something like:

fd_install_received()

and move the get_file() out of there so it has the same semantics as fd_install(). It seems rather dangerous to have a function like fd_install() that consumes a reference once it returned and another version of this that is basically the same thing but doesn't consume a reference because it takes its own. Seems an invitation for confusion. Does that make sense?

We have some competing opinions on this, I guess. What I really don't like is the copy/pasting of the get_unused_fd_flags() and put_unused_fd() needed by (nearly) all the callers. If it's a helper, it should help. Specifically, I'd like to see this:

int file_receive(int fd, unsigned long flags, struct file *file, int __user *fdptr)

I still fail to see what this whole put_user() handling buys us at all and why this function needs to be anymore complicated then simply:

fd_install_received(int fd, struct file *file) { security_file_receive(file); sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

fd_install(); return; }

exactly like fd_install() but for received files.

For scm you can fail somewhere in the middle of putting any number of file descriptors so you're left in a state with only a subset of requested file descriptors installed so it's not really useful there. And if you manage to install an fd but then fail to put_user() it userspace can simply check it's fds via proc and has to anyway on any scm message error. If you fail an scm message userspace better check their fds. For seccomp maybe but even there I doubt it and I still maintain that userspace screwing this up is on them which is how we do this most of the time. And for pidfd_getfd() this whole put_user() thing doesn't matter at all.

It's much easier and clearer if we simply have a fd_install() - fd_install_received() parallelism where we follow an established convention. _But_ if that blocks you from making this generic enough then at least the replace_fd() vs fd_install() logic seems it shouldn't be in there.

And the function name really needs to drive home the point that it installs an fd into the tasks fdtable no matter what version you go with. file_receive() is really not accurate enough for this at all.

...

{ struct socket *sock; int err;

err = security_file_receive(file); if (err) return err;

if (fd < 0) { /* Install new fd. */ int new_fd;

err = get_unused_fd_flags(flags);
if (err < 0)
	return err;
new_fd = err;

/* Copy fd to any waiting user memory. */
if (fdptr) {
	err = put_user(new_fd, fdptr);
	if (err < 0) {
		put_unused_fd(new_fd);
		return err;
	}
}
fd_install(new_fd, get_file(file));
fd = new_fd;

} else { /* Replace existing fd. */ err = replace_fd(fd, file, flags); if (err) return err; }

/* Bump the cgroup usage counts. */ sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

return fd; }

If everyone else *really* prefers keeping the get_unused_fd_flags() / put_unused_fd() stuff outside the helper, then I guess I'll give up, but I think it is MUCH cleaner this way -- all 4 users trim down lots of code duplication.

-- Kees Cook

How about this:

static int do_dup2(struct files_struct *files, struct file *file, unsigned fd, unsigned flags) __releases(&files->file_lock) { struct file *tofree; struct fdtable *fdt;

...

/* * New bit, allowing the file to be null. Doesn't have the same * "sanity check" bits from __alloc_fd */ if (likely(file)) get_file(file); rcu_assign_pointer(fdt->fd[fd], file);

__set_open_fd(fd, fdt);

... }

/* * File Receive - Receive a file from another process * * Encapsulates the logic to handle receiving a file from another task. It * does not install the file descriptor. That is delegated to the user. If * an error occurs that results in the file descriptor not being installed, * they must put_unused_fd. * * fd should be >= 0 if you intend on replacing a file descriptor, or * alternatively -1 if you want file_receive to allocate an FD for you * * Returns the fd number on success. * Returns negative error code on failure. * */ int file_receive(int fd, unsigned int flags, struct file *file) { int err; struct socket *sock; struct files_struct *files = current->files;

err = security_file_receive(file); if (err) return err;

if (fd >= 0) { if (fd >= rlimit(RLIMIT_NOFILE)) return -EBADF;

spin_lock(&files->file_lock); err = expand_files(files, fd); if (err < 0) { goto out_unlock; }

err = do_dup2(files, NULL, fd, flags); if (err) return err; } else { fd = get_unused_fd_flags(flags); if (fd < 0) return fd; }

sock = sock_from_file(file, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

return fd;

out_unlock: spin_unlock(&files->file_lock); return err; }

---

then the code in scm.c: err = file_receive(-1, flags, fp[i]); if (err < 0) break;

new_fd = err; err = put_user(new_fd, cmfptr); if (err) { put_unused_fd(new_fd); break; }

/* Bump the usage count and install the file. */ fd_install(new_fd, get_file(fp[i]));

And addfd: ret = file_receive(addfd->fd, addfd->flags, addfd->file); if (ret >= 0) fd_install(ret, get_file(addfd->file)); addfd->ret = ret;

----

This way there is: 1. No "put_user" logic in file_receive 2. Minimal (single) branching logic, unless there's something in between the file_receive and installing the FD, such as put_user. 3. Doesn't implement fd_install, so there's no ambiguity about it being file_install_received vs. just the receive logic.

On Tue, Jun 09, 2020 at 12:43:48PM -0700, Kees Cook wrote:

On Fri, Jun 05, 2020 at 07:54:36AM +0000, Sargun Dhillon wrote:

...

On Thu, Jun 04, 2020 at 02:52:26PM +0200, Christian Brauner wrote:

...

On Wed, Jun 03, 2020 at 07:22:57PM -0700, Kees Cook wrote:

...

On Thu, Jun 04, 2020 at 03:24:52AM +0200, Christian Brauner wrote:

...

On Tue, Jun 02, 2020 at 06:10:41PM -0700, Sargun Dhillon wrote:

...

Previously there were two chunks of code where the logic to receive file descriptors was duplicated in net. The compat version of copying file descriptors via SCM_RIGHTS did not have logic to update cgroups. Logic to change the cgroup data was added in: commit 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly") commit d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")

This was not copied to the compat path. This commit fixes that, and thus should be cherry-picked into stable.

This introduces a helper (file_receive) which encapsulates the logic for handling calling security hooks as well as manipulating cgroup information. This helper can then be used other places in the kernel where file descriptors are copied between processes

I tested cgroup classid setting on both the compat (x32) path, and the native path to ensure that when moving the file descriptor the classid is set.

Signed-off-by: Sargun Dhillon sargun@sargun.me Suggested-by: Kees Cook keescook@chromium.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: Christian Brauner christian.brauner@ubuntu.com Cc: Daniel Wagner daniel.wagner@bmw-carit.de Cc: David S. Miller davem@davemloft.net Cc: Jann Horn jannh@google.com, Cc: John Fastabend john.r.fastabend@intel.com Cc: Tejun Heo tj@kernel.org Cc: Tycho Andersen tycho@tycho.ws Cc: stable@vger.kernel.org Cc: cgroups@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org

---

fs/file.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/file.h | 1 + net/compat.c | 10 +++++----- net/core/scm.c | 14 ++++---------- 4 files changed, 45 insertions(+), 15 deletions(-)

This is all just a remote version of fd_install(), yet it deviates from fd_install()'s semantics and naming. That's not great imho. What about naming this something like:

fd_install_received()

and move the get_file() out of there so it has the same semantics as fd_install(). It seems rather dangerous to have a function like fd_install() that consumes a reference once it returned and another version of this that is basically the same thing but doesn't consume a reference because it takes its own. Seems an invitation for confusion. Does that make sense?

We have some competing opinions on this, I guess. What I really don't like is the copy/pasting of the get_unused_fd_flags() and put_unused_fd() needed by (nearly) all the callers. If it's a helper, it should help. Specifically, I'd like to see this:

int file_receive(int fd, unsigned long flags, struct file *file, int __user *fdptr)

I still fail to see what this whole put_user() handling buys us at all and why this function needs to be anymore complicated then simply:

fd_install_received(int fd, struct file *file) { security_file_receive(file); sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

fd_install(); return; }

exactly like fd_install() but for received files.

For scm you can fail somewhere in the middle of putting any number of file descriptors so you're left in a state with only a subset of requested file descriptors installed so it's not really useful there. And if you manage to install an fd but then fail to put_user() it userspace can simply check it's fds via proc and has to anyway on any scm message error. If you fail an scm message userspace better check their fds. For seccomp maybe but even there I doubt it and I still maintain that userspace screwing this up is on them which is how we do this most of the time. And for pidfd_getfd() this whole put_user() thing doesn't matter at all.

It's much easier and clearer if we simply have a fd_install() - fd_install_received() parallelism where we follow an established convention. _But_ if that blocks you from making this generic enough then at least the replace_fd() vs fd_install() logic seems it shouldn't be in there.

And the function name really needs to drive home the point that it installs an fd into the tasks fdtable no matter what version you go with. file_receive() is really not accurate enough for this at all.

...

{ struct socket *sock; int err;

err = security_file_receive(file); if (err) return err;

if (fd < 0) { /* Install new fd. */ int new_fd;

err = get_unused_fd_flags(flags);
if (err < 0)
	return err;
new_fd = err;

/* Copy fd to any waiting user memory. */
if (fdptr) {
	err = put_user(new_fd, fdptr);
	if (err < 0) {
		put_unused_fd(new_fd);
		return err;
	}
}
fd_install(new_fd, get_file(file));
fd = new_fd;

} else { /* Replace existing fd. */ err = replace_fd(fd, file, flags); if (err) return err; }

/* Bump the cgroup usage counts. */ sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

return fd; }

If everyone else *really* prefers keeping the get_unused_fd_flags() / put_unused_fd() stuff outside the helper, then I guess I'll give up, but I think it is MUCH cleaner this way -- all 4 users trim down lots of code duplication.

-- Kees Cook

How about this:

static int do_dup2(struct files_struct *files, struct file *file, unsigned fd, unsigned flags) __releases(&files->file_lock) { struct file *tofree; struct fdtable *fdt;

...

/* * New bit, allowing the file to be null. Doesn't have the same * "sanity check" bits from __alloc_fd */ if (likely(file)) get_file(file); rcu_assign_pointer(fdt->fd[fd], file);

__set_open_fd(fd, fdt);

IIUC, this means we can get the fdt into a state of an open fd with a NULL file... is that okay? That feels like something Al might rebel at. :)

...

... }

/*

• File Receive - Receive a file from another process
• Encapsulates the logic to handle receiving a file from another task. It
• does not install the file descriptor. That is delegated to the user. If
• an error occurs that results in the file descriptor not being installed,
• they must put_unused_fd.
• fd should be >= 0 if you intend on replacing a file descriptor, or
• alternatively -1 if you want file_receive to allocate an FD for you
• Returns the fd number on success.
• Returns negative error code on failure.

*/ int file_receive(int fd, unsigned int flags, struct file *file) { int err; struct socket *sock; struct files_struct *files = current->files;

err = security_file_receive(file); if (err) return err;

if (fd >= 0) { if (fd >= rlimit(RLIMIT_NOFILE)) return -EBADF;

spin_lock(&files->file_lock);
err = expand_files(files, fd);
if (err < 0) {
	goto out_unlock;
}

err = do_dup2(files, NULL, fd, flags);
if (err)
	return err;

This seems like we're duplicating some checks and missing others -- I really think we need to do this using the existing primitives. But I'd really like some review or commentary from Al. We can do this a bunch of ways, and I'd like to know which way looks best to him. :(

...

This way there is:

1. No "put_user" logic in file_receive
2. Minimal (single) branching logic, unless there's something in between the file_receive and installing the FD, such as put_user.
3. Doesn't implement fd_install, so there's no ambiguity about it being file_install_received vs. just the receive logic.

I still wonder if we should refactor SCM_RIGHTS to just delay put_user failures, which would simplify a bunch. It's a behavior change, but it

I'm looking at __scm_install_fd() and I wonder what specifically you mean by that? The put_user() seems to be placed such that the install occurrs only if it succeeded. Sure, it only handles a single fd but whatever. Userspace knows that already. Just look at systemd when a msg fails:

void cmsg_close_all(struct msghdr *mh) { struct cmsghdr *cmsg;

assert(mh);

CMSG_FOREACH(cmsg, mh) if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS) close_many((int*) CMSG_DATA(cmsg), (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int)); }

The only reasonable scenario for this whole mess I can think of is sm like (pseudo code):

fd_install_received(int fd, struct file *file) { sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

fd_install(); }

error = 0; fdarray = malloc(fdmax); for (i = 0; i < fdmax; i++) { fdarray[i] = get_unused_fd_flags(o_flags); if (fdarray[i] < 0) { error = -EBADF; break; }

error = security_file_receive(file); if (error) break;

error = put_user(fd_array[i], ufd); if (error) break; }

for (i = 0; i < fdmax; i++) { if (error) { /* ignore errors */ put_user(-EBADF, ufd); /* If this put_user() fails and the first one succeeded userspace might now close an fd it didn't intend to. */ put_unused_fd(fdarray[i]); } else { fd_install_received(fdarray[i], file); } }

Christian

On June 9, 2020 10:55:42 PM GMT+02:00, Kees Cook keescook@chromium.org wrote:

On Tue, Jun 09, 2020 at 10:03:46PM +0200, Christian Brauner wrote:

...

I'm looking at __scm_install_fd() and I wonder what specifically you mean by that? The put_user() seems to be placed such that the install occurrs only if it succeeded. Sure, it only handles a single fd but whatever. Userspace knows that already. Just look at systemd when a

msg

...

fails:

void cmsg_close_all(struct msghdr *mh) { struct cmsghdr *cmsg;

    assert(mh);

    CMSG_FOREACH(cmsg, mh)
            if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type

== SCM_RIGHTS)

...

                    close_many((int*) CMSG_DATA(cmsg),

(cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int));

...

}

The only reasonable scenario for this whole mess I can think of is sm

like (pseudo code):

...

fd_install_received(int fd, struct file *file) { sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

fd_install(); }

error = 0; fdarray = malloc(fdmax); for (i = 0; i < fdmax; i++) { fdarray[i] = get_unused_fd_flags(o_flags); if (fdarray[i] < 0) { error = -EBADF; break; }

error = security_file_receive(file); if (error) break;

error = put_user(fd_array[i], ufd); if (error) break; }

for (i = 0; i < fdmax; i++) { if (error) { /* ignore errors */ put_user(-EBADF, ufd); /* If this put_user() fails and the first

one succeeded userspace might now close an fd it didn't intend to. */

...

put_unused_fd(fdarray[i]);

} else { fd_install_received(fdarray[i], file); } }

I see 4 cases of the same code pattern (get_unused_fd_flags(), sock_update_*(), fd_install()), one of them has this difficult put_user() in the middle, and one of them has a potential replace_fd() instead of the get_used/fd_install. So, to me, it makes sense to have a helper that encapsulates the common work that each of those call sites has to do, which I keep cringing at all these suggestions that leave portions of it outside the helper.

If it's too ugly to keep the put_user() in the helper, then we can try what was suggested earlier, and just totally rework the failure path for SCM_RIGHTS.

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

Christian

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

-- 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... [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/incl...

On Wed, Jun 10, 2020 at 08:48:45AM +0000, David Laight wrote:

From: Sargun Dhillon

...

Sent: 10 June 2020 09:13

In essence the 'copy_to_user' is done by the wrapper code. The code filling in the CMSG buffer can be considered to be writing a kernel buffer.

IIRC other kernels (eg NetBSD) do the copies for ioctl() requests in the ioctl syscall wrapper. The IOW/IOR/IOWR flags have to be right.

Yeah, this seems like it'd make a lot more sense (and would have easily caught the IOR/IOW issue pointed out later in the thread). I wonder how insane it would be to try to fix that globally in the kernel...

On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote:

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

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

Ah, sorry, I missed this before I posted my "v3.2" tree link.

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?

Hmmm. I see what you mean -- SCM_RIGHTS effectively _requires_ a valid __user pointer, so we can't use NULL to indicate "we don't want this". I'm not sure I can pass this through directly at all, though.

---

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.

Okay, sounds good.

---

+#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."

Okay, let me tweak things and get a "v3.3". ;)

On Thu, Jun 11, 2020 at 11:06:31AM +0000, Sargun Dhillon wrote:

On Thu, Jun 11, 2020 at 12:01:14PM +0200, Christian Brauner wrote:

...

On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote:

...

On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote:

...

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.

Yeah, that seems reasonable. Here's the diff for that part:

Why does it matter that the ioctl() has the size of the struct embedded within? Afaik, the kernel itself doesn't do anything with that size. It merely checks that the size is not pathological and it does so at compile time.

#ifdef __CHECKER__ #define _IOC_TYPECHECK(t) (sizeof(t)) #else /* provoke compile error for invalid uses of size argument */ extern unsigned int __invalid_size_argument_for_IOC; #define _IOC_TYPECHECK(t) \ ((sizeof(t) == sizeof(t[1]) && \ sizeof(t) < (1 << _IOC_SIZEBITS)) ? \ sizeof(t) : __invalid_size_argument_for_IOC) #endif

The size itself is not verified at runtime. copy_struct_from_user() still makes sense at least if we're going to allow expanding the struct in the future.

Right, but if we simply change our headers and extend the struct, it will break all existing programs compiled against those headers. In order to avoid that, if we intend on extending this struct by appending to it, we need to have a backwards compatibility mechanism. Just having copy_struct_from_user isn't enough. The data structure either must be fixed size, or we need a way to handle multiple ioctl numbers derived from headers with different sized struct arguments

The two approaches I see are:

1. use more indirection. This has previous art in drm[1]. That's look

something like this:

struct seccomp_notif_addfd_ptr { __u64 size; __u64 addr; }

... And then it'd be up to us to dereference the addr and copy struct from user.

Which isn't great but could do.

2. Expose one ioctl to the user, many internally

e.g., public api:

struct seccomp_notif { __u64 id; __u64 pid; struct seccomp_data; __u64 fancy_new_field; }

#define SECCOMP_IOCTL_NOTIF_RECV SECCOMP_IOWR(0, struct seccomp_notif)

internally: struct seccomp_notif_v1 { __u64 id; __u64 pid; struct seccomp_data; }

struct seccomp_notif_v2 { __u64 id; __u64 pid; struct seccomp_data; __u64 fancy_new_field; }

and we can switch like this: switch (cmd) { /* for example. We actually have to do this for any struct we intend to * extend to get proper backwards compatibility */ case SECCOMP_IOWR(0, struct seccomp_notif_v1) return seccomp_notify_recv(filter, buf, sizeof(struct seccomp_notif_v1)); case SECCOMP_IOWR(0, struct seccomp_notif_v2) return seccomp_notify_recv(filter, buf, sizeof(struct seccomp_notif_v3)); ... case SECCOMP_IOCTL_NOTIF_SEND: return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); default: return -EINVAL; }

This has the downside that programs compiled against more modern kernel headers will break on older kernels.

3. We can take the approach you suggested.

#define UNSIZED(cmd) (cmd & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT) static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct seccomp_filter *filter = file->private_data; void __user *buf = (void __user *)arg; int size = _IOC_SIZE(cmd); cmd = UNSIZED(cmd);

switch (cmd) { /* for example. We actually have to do this for any struct we intend to * extend to get proper backwards compatibility */ case UNSIZED(SECCOMP_IOCTL_NOTIF_RECV): return seccomp_notify_recv(filter, buf, size); ... case SECCOMP_IOCTL_NOTIF_SEND: return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); default: return -EINVAL; } }

...

Leaving that aside, the proposed direction here seems to mean that any change to the struct itself will immediately mean a new ioctl() but afaict, that also means a new struct. Since when you simply extend the struct for the sake of the new ioctl you also change the size for the ioctl.

Sure, you can simply treat the struct coming through the old ioctl as being "capped" by e.g. hiding the size as suggested but then the gain by having two separate ioctls is 0 compared to simply versioning the struct with an explicit size member since the size encoded in the ioctl and the actual size of the struct don't line up anymore which is the only plus I can see for relying on _IOC_SIZE(). All this manages to do then is to make it more annoying for userspace since they now need to maintain multiple ioctls(). And if you have - however unlikely - say three different ioctls all to be used with a different struct size of the same struct I now need to know which ioctl() goes with which size of the struct (I guess you could append the size to the ioctl name? *shudder*). If you have the size in the struct itself you don't need to care about any of that. Maybe I'm not making sense or I misunderstand what's going on though.

Christian

I don't understand why userspace has to have any knowledge of this. As soon as we add the code above, and we use copy_struct_from_user based on _that_ size,

Hm, which code exactly?

In the previous mail the only thing proposed was to switch to a simple copy_from_user() which effectively bars us from extending the seccomp_addfd struct which this is about, right? At that point, the only option then becomes to either introduce a new ioctl() and a new struct or to go for the hack in e.g. 3. (Afaiu, 2. is not working anymore because we break userspace as soon as we append "fancy_new_field" to the struct because it changes the ioctl() unless I'm missing something.)

Let me maybe rephrase: I'd prefer we merge something for addfd that is extensible with minimal burden on userspace. But if we are fine with saying "we don't care, let's just use copy_from_user() for addfd and if we extend we add a new struct + ioctl" then ok, sure. But I would prefer to keep dealing with new structs + ioctls (Look at the end of btrfs.h [1] unlikely to be a problem for us, but still.) as little as possible because that will be more churn in userspace code than I'd prefer.

So either [1] or - since none of the generic extensibility seems to be particularly nice - we bite the bullet and just add a:

__u64 reserved[4]

field and hope that this will carry us for a long time (probably will for quite a long time) and defer the new ioctl() problem.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/incl...

userspace will get free upgrades. If they are compiling against an older header than the kernel, size will return a smaller number, and thus we will zero out our trailing bits, and if their number is bigger, we just check their bits are appropriately zeroed.

This approach would be forwards-and-backwards compatible.

There's a little bit of prior art here as well [2]. The approach is that we effectively do the thing we had earlier with passing a size with copy_struct_from_user, but instead of the size being embedded in the struct, it's embedded in the ioctl command itself.

That looks super sketchy. :D

Christian

On Thu, Jun 11, 2020 at 02:56:22PM +0000, David Laight wrote:

From: Sargun Dhillon

...

Sent: 11 June 2020 12:07 Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes

On Thu, Jun 11, 2020 at 12:01:14PM +0200, Christian Brauner wrote:

...

On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote:

...

On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote:

...

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.

Yeah, that seems reasonable. Here's the diff for that part:

Why does it matter that the ioctl() has the size of the struct embedded within? Afaik, the kernel itself doesn't do anything with that size. It merely checks that the size is not pathological and it does so at compile time.

#ifdef __CHECKER__ #define _IOC_TYPECHECK(t) (sizeof(t)) #else /* provoke compile error for invalid uses of size argument */ extern unsigned int __invalid_size_argument_for_IOC; #define _IOC_TYPECHECK(t) \ ((sizeof(t) == sizeof(t[1]) && \ sizeof(t) < (1 << _IOC_SIZEBITS)) ? \ sizeof(t) : __invalid_size_argument_for_IOC) #endif

The size itself is not verified at runtime. copy_struct_from_user() still makes sense at least if we're going to allow expanding the struct in the future.

Right, but if we simply change our headers and extend the struct, it will break all existing programs compiled against those headers. In order to avoid that, if we intend on extending this struct by appending to it, we need to have a backwards compatibility mechanism. Just having copy_struct_from_user isn't enough. The data structure either must be fixed size, or we need a way to handle multiple ioctl numbers derived from headers with different sized struct arguments

The two approaches I see are:

1. use more indirection. This has previous art in drm[1]. That's look

something like this:

struct seccomp_notif_addfd_ptr { __u64 size; __u64 addr; }

... And then it'd be up to us to dereference the addr and copy struct from user.

Do not go down that route. It isn't worth the pain.

You should also assume that userspace might have a compile-time check on the buffer length (I've written one - not hard) and that the kernel might (in the future - or on a BSD kernel) be doing the user copies for you.

Also, if you change the structure you almost certainly need to change the name of the ioctl cmd as well as its value. Otherwise a recompiled program will pass the new cmd value (and hopefully the right sized buffer) but it won't have initialised the buffer properly. This is likely to lead to unexpected behaviour.

Hmmm.

So, while initially I thought Sargun's observation about ioctl's fixed struct size was right, I think I've been swayed to Christian's view (which is supported by the long tail of struct size pain we've seen in other APIs).

Doing a separate ioctl for each structure version seems like the "old solution" now that we've got EA syscalls. So, I'd like to keep the size and copy_struct_from_user().

Which leaves us with the question of how to deal with the ioctl numbering. As we've seen, there is no actual enforcement of direction nor size, so to that end, while we could provide the hints about both, I guess we just don't need to. To that end, perhaps _IO() is best:

#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IO(3)

Alternatively, we could use a size of either 0, 8(u64), or -1, and then use IORW() so we _also_ won't paint ourselves into a corner if we ever want to write something back to userspace in the structure:

#define SECCOMP_IOCTL_EA(nr) _IOC(_IOC_READ|_IOC_WRITE,SECCOMP_IOC_MAGIC,(nr),0) #define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOCTL_EA(3)

or

#define SECCOMP_IOCTL_EA(nr) _IOC(_IOC_READ|_IOC_WRITE,SECCOMP_IOC_MAGIC,(nr),8) #define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOCTL_EA(3)

or

#define SECCOMP_IOCTL_EA(nr) _IOC(_IOC_READ|_IOC_WRITE,SECCOMP_IOC_MAGIC,(nr),_IOC_SIZEMASK) #define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOCTL_EA(3)

I think I prefer the last one.

From: Kees Cook

Sent: 12 June 2020 00:50

...

From: Sargun Dhillon

...

Sent: 11 June 2020 12:07 Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across

processes

...

...

On Thu, Jun 11, 2020 at 12:01:14PM +0200, Christian Brauner wrote:

...

On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote:

...

On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote:

...

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.

Yeah, that seems reasonable. Here's the diff for that part:

Why does it matter that the ioctl() has the size of the struct embedded within? Afaik, the kernel itself doesn't do anything with that size. It merely checks that the size is not pathological and it does so at compile time.

#ifdef __CHECKER__ #define _IOC_TYPECHECK(t) (sizeof(t)) #else /* provoke compile error for invalid uses of size argument */ extern unsigned int __invalid_size_argument_for_IOC; #define _IOC_TYPECHECK(t) \ ((sizeof(t) == sizeof(t[1]) && \ sizeof(t) < (1 << _IOC_SIZEBITS)) ? \ sizeof(t) : __invalid_size_argument_for_IOC) #endif

The size itself is not verified at runtime. copy_struct_from_user() still makes sense at least if we're going to allow expanding the struct in the future.

Right, but if we simply change our headers and extend the struct, it will break all existing programs compiled against those headers. In order to avoid that, if we intend on extending this struct by appending to it, we need to have a backwards compatibility mechanism. Just having copy_struct_from_user isn't enough. The data structure either must be fixed size, or we need a way to handle multiple ioctl numbers derived from headers with different sized struct arguments

The two approaches I see are:

1. use more indirection. This has previous art in drm[1]. That's look

something like this:

struct seccomp_notif_addfd_ptr { __u64 size; __u64 addr; }

... And then it'd be up to us to dereference the addr and copy struct from user.

Do not go down that route. It isn't worth the pain.

You should also assume that userspace might have a compile-time check on the buffer length (I've written one - not hard) and that the kernel might (in the future - or on a BSD kernel) be doing the user copies for you.

Also, if you change the structure you almost certainly need to change the name of the ioctl cmd as well as its value. Otherwise a recompiled program will pass the new cmd value (and hopefully the right sized buffer) but it won't have initialised the buffer properly. This is likely to lead to unexpected behaviour.

Hmmm.

So, while initially I thought Sargun's observation about ioctl's fixed struct size was right, I think I've been swayed to Christian's view (which is supported by the long tail of struct size pain we've seen in other APIs).

Doing a separate ioctl for each structure version seems like the "old solution" now that we've got EA syscalls. So, I'd like to keep the size and copy_struct_from_user().

If the size is variable then why not get the application to fill in the size of the structure it is sending at the time of the ioctl.

So you'd have: #define xxx_IOCTL_17(param) _IOCW('X', 17, sizeof *(param))

The application code would then do: ioctl(fd, xxx_IOCTL_17(arg), arg);

The kernel code can either choose to have specific 'case' for each size, or mask off the length bits and do the length check later.

David

- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

On Fri, Jun 12, 2020 at 10:46:30AM +0000, Sargun Dhillon wrote:

My suggest, written out (no idea if this code actually works), is as follows:

ioctl.h: /* This needs to be added */ #define IOCDIR_MASK (_IOC_DIRMASK << _IOC_DIRSHIFT)

This exists already:

#define _IOC_DIRMASK ((1 << _IOC_DIRBITS)-1)

seccomp.h:

struct struct seccomp_notif_addfd { __u64 fd; ... }

/* or IOW? */ #define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOWR(3, struct seccomp_notif_addfd)

seccomp.c: static long seccomp_notify_addfd(struct seccomp_filter *filter, struct seccomp_notif_addfd __user *uaddfd int size) { struct seccomp_notif_addfd addfd; int ret;

if (size < 32) return -EINVAL; if (size > PAGE_SIZE) return -E2BIG;

(Tanget: what was the reason for copy_struct_from_user() not including the min/max check? I have a memory of Al objecting to having an "internal" limit?)

ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); if (ret) return ret;

... }

/* Mask out size */ #define SIZE_MASK(cmd) (~IOCSIZE_MASK & cmd)

/* Mask out direction */ #define DIR_MASK(cmd) (~IOCDIR_MASK & cmd)

static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct seccomp_filter *filter = file->private_data; void __user *buf = (void __user *)arg;

/* Fixed size ioctls. Can be converted later on? */ switch (cmd) { case SECCOMP_IOCTL_NOTIF_RECV: return seccomp_notify_recv(filter, buf); case SECCOMP_IOCTL_NOTIF_SEND: return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); }

/* Probably should make some nicer macros here */ switch (SIZE_MASK(DIR_MASK(cmd))) { case SIZE_MASK(DIR_MASK(SECCOMP_IOCTL_NOTIF_ADDFD)):

Ah yeah, I like this because of what you mention below: it's forward compat too. (I'd just use the ioctl masks directly...)

switch (cmd & ~(_IOC_SIZEMASK | _IOC_DIRMASK))

return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));

I really like that this ends up having the same construction as a standard EA syscall: the size is part of the syscall arguments.

default: return -EINVAL; } }

---

What boxes does this tick?

• Forwards (and backwards) compatibility
• Applies to existing commands
• Command can be extended without requiring new ioctl to be defined

(Technically, a new one is always redefined, but it's automatic in that the kernel needs to do nothing.)

• It well accomodates the future where we want to have a kernel helper copy the structures from userspace

Yeah, this is a good solution.

The fact that the size of the argument struct, and the ioctl are defined in the same header gives us the ability to "cheat", and for the argument size to be included / embedded for free in the command passed to ioctl. In turn, this gives us two benefits. First, it means we don't have to copy from user twice, and can just do it all in one shot since the size is passed with the syscall arguments. Second, it means that the user does not have to do the following:

seccomp_notif_addfd addfd = {}; addfd.size = sizeof(struct seccomp_notif_addfd)

Because sizeof(struct seccomp_notif_addfd) is embedded in SECCOMP_IOCTL_NOTIF_ADDFD based on the same headers they plucked the struct out of.

Cool. I will do more patch reworking! ;)

On Fri, Jun 12, 2020 at 08:13:25AM -0700, Kees Cook wrote:

On Fri, Jun 12, 2020 at 10:46:30AM +0000, Sargun Dhillon wrote:

...

My suggest, written out (no idea if this code actually works), is as follows:

ioctl.h: /* This needs to be added */ #define IOCDIR_MASK (_IOC_DIRMASK << _IOC_DIRSHIFT)

This exists already:

#define _IOC_DIRMASK ((1 << _IOC_DIRBITS)-1)

...

seccomp.h:

struct struct seccomp_notif_addfd { __u64 fd; ... }

/* or IOW? */ #define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOWR(3, struct seccomp_notif_addfd)

seccomp.c: static long seccomp_notify_addfd(struct seccomp_filter *filter, struct seccomp_notif_addfd __user *uaddfd int size) { struct seccomp_notif_addfd addfd; int ret;

if (size < 32) return -EINVAL; if (size > PAGE_SIZE) return -E2BIG;

(Tanget: what was the reason for copy_struct_from_user() not including the min/max check? I have a memory of Al objecting to having an "internal" limit?)

Al didn't want the PAGE_SIZE limit in there because there's nothing inherently wrong with copying insane amounts of memory.

(Another tangent. I've asked this on Twitter not too long ago: do we have stats how long copy_from_user()/copy_struct_from_user() takes with growing struct/memory size? I'd be really interested in this. I have a feeling that clone3()'s and - having had a chat with David Howells - openat2()'s structs will continue to grow for a while... and I'd really like to have some numbers on when copy_struct_from_user() becomes costly or how costly it becomes.)

...

ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); if (ret) return ret;

... }

/* Mask out size */ #define SIZE_MASK(cmd) (~IOCSIZE_MASK & cmd)

/* Mask out direction */ #define DIR_MASK(cmd) (~IOCDIR_MASK & cmd)

static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct seccomp_filter *filter = file->private_data; void __user *buf = (void __user *)arg;

/* Fixed size ioctls. Can be converted later on? */ switch (cmd) { case SECCOMP_IOCTL_NOTIF_RECV: return seccomp_notify_recv(filter, buf); case SECCOMP_IOCTL_NOTIF_SEND: return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); }

/* Probably should make some nicer macros here */ switch (SIZE_MASK(DIR_MASK(cmd))) { case SIZE_MASK(DIR_MASK(SECCOMP_IOCTL_NOTIF_ADDFD)):

Ah yeah, I like this because of what you mention below: it's forward compat too. (I'd just use the ioctl masks directly...)

switch (cmd & ~(_IOC_SIZEMASK | _IOC_DIRMASK))

...

return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));

I really like that this ends up having the same construction as a standard EA syscall: the size is part of the syscall arguments.

This is basically what I had proposed in my previous mail, right?

Christian

On Fri, Jun 12, 2020 at 11:38:33AM -0700, Kees Cook wrote:

On Fri, Jun 12, 2020 at 08:28:16PM +0200, Christian Brauner wrote:

...

Al didn't want the PAGE_SIZE limit in there because there's nothing inherently wrong with copying insane amounts of memory.

Right, ok.

...

(Another tangent. I've asked this on Twitter not too long ago: do we have stats how long copy_from_user()/copy_struct_from_user() takes with growing struct/memory size? I'd be really interested in this. I have a feeling that clone3()'s and - having had a chat with David Howells - openat2()'s structs will continue to grow for a while... and I'd really like to have some numbers on when copy_struct_from_user() becomes costly or how costly it becomes.)

How long it takes? It should be basically the same, the costs should be mostly in switching memory protections, etc. I wouldn't imagine how many bytes being copied would matter much here, given the sub-page sizes.

This makes me _very_ happy.

Christian

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:

...

On Tue, Jun 09, 2020 at 10:03:46PM +0200, Christian Brauner wrote:

...

I'm looking at __scm_install_fd() and I wonder what specifically you mean by that? The put_user() seems to be placed such that the install occurrs only if it succeeded. Sure, it only handles a single fd but whatever. Userspace knows that already. Just look at systemd when a

msg

...

fails:

void cmsg_close_all(struct msghdr *mh) { struct cmsghdr *cmsg;

    assert(mh);

    CMSG_FOREACH(cmsg, mh)
            if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type

== SCM_RIGHTS)

...

                    close_many((int*) CMSG_DATA(cmsg),

(cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int));

...

}

The only reasonable scenario for this whole mess I can think of is sm

like (pseudo code):

...

fd_install_received(int fd, struct file *file) { sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); }

fd_install(); }

error = 0; fdarray = malloc(fdmax); for (i = 0; i < fdmax; i++) { fdarray[i] = get_unused_fd_flags(o_flags); if (fdarray[i] < 0) { error = -EBADF; break; }

error = security_file_receive(file); if (error) break;

error = put_user(fd_array[i], ufd); if (error) break; }

for (i = 0; i < fdmax; i++) { if (error) { /* ignore errors */ put_user(-EBADF, ufd); /* If this put_user() fails and the first

one succeeded userspace might now close an fd it didn't intend to. */

...

put_unused_fd(fdarray[i]);

} else { fd_install_received(fdarray[i], file); } }

I see 4 cases of the same code pattern (get_unused_fd_flags(), sock_update_*(), fd_install()), one of them has this difficult put_user() in the middle, and one of them has a potential replace_fd() instead of the get_used/fd_install. So, to me, it makes sense to have a helper that encapsulates the common work that each of those call sites has to do, which I keep cringing at all these suggestions that leave portions of it outside the helper.

If it's too ugly to keep the put_user() in the helper, then we can try what was suggested earlier, and just totally rework the failure path for SCM_RIGHTS.

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

For the record, as I didn't see this yesterday since I was already looking at a kernel with Christoph's changes. His changes just move the logic that was already there before into a separate helper.

So effectively nothing has changed semantically in the scm code at all.

This is why I was asking yesterday what you meant by reworking the scm code's put_user() logic as it seems obviously correct as it is done now.

Christian