linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Use __scm_install_fd() more widely
@ 2020-06-10  4:52 Kees Cook
  2020-06-10  4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10  4:52 UTC (permalink / raw)
  To: David S. Miller
  Cc: Kees Cook, Christoph Hellwig, Christian Brauner, Sargun Dhillon,
	Jakub Kicinski, netdev, linux-kernel

Hi,

This extends the recent work hch did for scm_detach_fds(), and updates
the compat path as well, fixing bugs in the process. Additionally,
an effectively incomplete and open-coded __scm_install_fd() is fixed
in pidfd_getfd().

Thanks!

-Kees

Kees Cook (2):
  net/scm: Regularize compat handling of scm_detach_fds()
  pidfd: Replace open-coded partial __scm_install_fd()

 include/net/scm.h |  1 +
 kernel/pid.c      | 12 ++---------
 net/compat.c      | 55 +++++++++++++++++++++--------------------------
 net/core/scm.c    | 43 +++++++++++++++++++++++-------------
 4 files changed, 56 insertions(+), 55 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds()
  2020-06-10  4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
@ 2020-06-10  4:52 ` Kees Cook
  2020-06-10  4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
  2020-06-10  9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
  2 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10  4:52 UTC (permalink / raw)
  To: David S. Miller
  Cc: Kees Cook, Christoph Hellwig, Christian Brauner, Sargun Dhillon,
	Jakub Kicinski, netdev, linux-kernel

Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
scm_detach_fds") into the compat code.

Also moves the check added in commit 1f466e1f15cf ("net: cleanly handle
kernel vs user buffers for ->msg_control") to before the compat call, even
though it should be impossible for an in-kernel call to also be compat.

Regularized any remaining differences, including a whitespace issue, a
checkpatch warning, and adding the check from commit 6900317f5eff ("net,
scm: fix PaX detected msg_controllen overflow in scm_detach_fds") which
fixed an overflow unique to 64-bit. To avoid confusion when comparing
the compat handler to the native handler, just include the same check
in the compat handler.

Fixes: 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
Fixes: d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
AFAICT, the following patches are needed for back-porting this to stable:

0462b6bdb644 ("net: add a CMSG_USER_DATA macro")
2618d530dd8b ("net/scm: cleanup scm_detach_fds")
1f466e1f15cf ("net: cleanly handle kernel vs user buffers for ->msg_control")
6e8a4f9dda38 ("net: ignore sock_from_file errors in __scm_install_fd")
---
 include/net/scm.h |  1 +
 net/compat.c      | 55 +++++++++++++++++++++--------------------------
 net/core/scm.c    | 16 +++++++-------
 3 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/include/net/scm.h b/include/net/scm.h
index 1ce365f4c256..4dcc766f15b3 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -37,6 +37,7 @@ struct scm_cookie {
 #endif
 };
 
+int __scm_install_fd(struct file *file, int __user *ufd, int o_flags);
 void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm);
 void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm);
 int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm);
diff --git a/net/compat.c b/net/compat.c
index 5e3041a2c37d..117f1869bf3b 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -281,39 +281,31 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *dat
 	return 0;
 }
 
-void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm)
+static int scm_max_fds_compat(struct msghdr *msg)
 {
-	struct compat_cmsghdr __user *cm = (struct compat_cmsghdr __user *) kmsg->msg_control;
-	int fdmax = (kmsg->msg_controllen - sizeof(struct compat_cmsghdr)) / sizeof(int);
-	int fdnum = scm->fp->count;
-	struct file **fp = scm->fp->fp;
-	int __user *cmfptr;
-	int err = 0, i;
+	if (msg->msg_controllen <= sizeof(struct compat_cmsghdr))
+		return 0;
+	return (msg->msg_controllen - sizeof(struct compat_cmsghdr)) / sizeof(int);
+}
 
-	if (fdnum < fdmax)
-		fdmax = fdnum;
+void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
+{
+	struct compat_cmsghdr __user *cm =
+		(struct compat_cmsghdr __user *)msg->msg_control;
+	int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
+	int fdmax = min_t(int, scm_max_fds_compat(msg), scm->fp->count);
+	int __user *cmsg_data = CMSG_USER_DATA(cm);
+	int err = 0, i;
 
-	for (i = 0, cmfptr = (int __user *) CMSG_COMPAT_DATA(cm); i < fdmax; i++, cmfptr++) {
-		int new_fd;
-		err = security_file_receive(fp[i]);
+	for (i = 0; i < fdmax; i++) {
+		err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
 		if (err)
 			break;
-		err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & kmsg->msg_flags
-					  ? O_CLOEXEC : 0);
-		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]));
 	}
 
 	if (i > 0) {
 		int cmlen = CMSG_COMPAT_LEN(i * sizeof(int));
+
 		err = put_user(SOL_SOCKET, &cm->cmsg_level);
 		if (!err)
 			err = put_user(SCM_RIGHTS, &cm->cmsg_type);
@@ -321,16 +313,19 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm)
 			err = put_user(cmlen, &cm->cmsg_len);
 		if (!err) {
 			cmlen = CMSG_COMPAT_SPACE(i * sizeof(int));
-			kmsg->msg_control += cmlen;
-			kmsg->msg_controllen -= cmlen;
+			if (msg->msg_controllen < cmlen)
+				cmlen = msg->msg_controllen;
+			msg->msg_control += cmlen;
+			msg->msg_controllen -= cmlen;
 		}
 	}
-	if (i < fdnum)
-		kmsg->msg_flags |= MSG_CTRUNC;
+
+	if (i < scm->fp->count || (scm->fp->count && fdmax <= 0))
+		msg->msg_flags |= MSG_CTRUNC;
 
 	/*
-	 * All of the files that fit in the message have had their
-	 * usage counts incremented, so we just free the list.
+	 * All of the files that fit in the message have had their usage counts
+	 * incremented, so we just free the list.
 	 */
 	__scm_destroy(scm);
 }
diff --git a/net/core/scm.c b/net/core/scm.c
index 875df1c2989d..86d96152646f 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -280,7 +280,7 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
 }
 EXPORT_SYMBOL(put_cmsg_scm_timestamping);
 
-static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
+int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
 {
 	struct socket *sock;
 	int new_fd;
@@ -319,29 +319,29 @@ static int scm_max_fds(struct msghdr *msg)
 
 void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 {
-	struct cmsghdr __user *cm
-		= (__force struct cmsghdr __user*)msg->msg_control;
+	struct cmsghdr __user *cm =
+		(__force struct cmsghdr __user*)msg->msg_control;
 	int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
 	int fdmax = min_t(int, scm_max_fds(msg), scm->fp->count);
 	int __user *cmsg_data = CMSG_USER_DATA(cm);
 	int err = 0, i;
 
+	/* no use for FD passing from kernel space callers */
+	if (WARN_ON_ONCE(!msg->msg_control_is_user))
+		return;
+
 	if (msg->msg_flags & MSG_CMSG_COMPAT) {
 		scm_detach_fds_compat(msg, scm);
 		return;
 	}
 
-	/* no use for FD passing from kernel space callers */
-	if (WARN_ON_ONCE(!msg->msg_control_is_user))
-		return;
-
 	for (i = 0; i < fdmax; i++) {
 		err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
 		if (err)
 			break;
 	}
 
-	if (i > 0)  {
+	if (i > 0) {
 		int cmlen = CMSG_LEN(i * sizeof(int));
 
 		err = put_user(SOL_SOCKET, &cm->cmsg_level);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd()
  2020-06-10  4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
  2020-06-10  4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
@ 2020-06-10  4:52 ` Kees Cook
  2020-06-10 16:45   ` Sargun Dhillon
  2020-06-10  9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
  2 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2020-06-10  4:52 UTC (permalink / raw)
  To: David S. Miller
  Cc: Kees Cook, Christoph Hellwig, Christian Brauner, Sargun Dhillon,
	Jakub Kicinski, netdev, linux-kernel

The sock counting (sock_update_netprioidx() and sock_update_classid())
was missing from this implementation of fd installation, compared to
SCM_RIGHTS. Use the new scm helper to get the work done, after adjusting
it to return the installed fd and accept a NULL user pointer.

Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
AFAICT, the following patches are needed for back-porting this to stable:

0462b6bdb644 ("net: add a CMSG_USER_DATA macro")
2618d530dd8b ("net/scm: cleanup scm_detach_fds")
1f466e1f15cf ("net: cleanly handle kernel vs user buffers for ->msg_control")
6e8a4f9dda38 ("net: ignore sock_from_file errors in __scm_install_fd")
---
 kernel/pid.c   | 12 ++----------
 net/compat.c   |  2 +-
 net/core/scm.c | 27 ++++++++++++++++++++-------
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index f1496b757162..a7ce4ba898d3 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -42,6 +42,7 @@
 #include <linux/sched/signal.h>
 #include <linux/sched/task.h>
 #include <linux/idr.h>
+#include <net/scm.h>
 
 struct pid init_struct_pid = {
 	.count		= REFCOUNT_INIT(1),
@@ -635,18 +636,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 = __scm_install_fd(file, NULL, O_CLOEXEC);
 	if (ret < 0)
 		fput(file);
-	else
-		fd_install(ret, file);
-
 	return ret;
 }
 
diff --git a/net/compat.c b/net/compat.c
index 117f1869bf3b..f8575555b6d7 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -299,7 +299,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
 
 	for (i = 0; i < fdmax; i++) {
 		err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
-		if (err)
+		if (err < 0)
 			break;
 	}
 
diff --git a/net/core/scm.c b/net/core/scm.c
index 86d96152646f..e80648fb4da7 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -280,6 +280,14 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
 }
 EXPORT_SYMBOL(put_cmsg_scm_timestamping);
 
+/**
+ * __scm_install_fd() - Install received file into file descriptor table
+ *
+ * Installs a received file into the file descriptor table, with appropriate
+ * checks and count updates.
+ *
+ * Returns fd installed or -ve on error.
+ */
 int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
 {
 	struct socket *sock;
@@ -294,20 +302,25 @@ int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
 	if (new_fd < 0)
 		return new_fd;
 
-	error = put_user(new_fd, ufd);
-	if (error) {
-		put_unused_fd(new_fd);
-		return error;
+	if (ufd) {
+		error = put_user(new_fd, ufd);
+		if (error) {
+			put_unused_fd(new_fd);
+			return error;
+		}
 	}
 
-	/* Bump the usage count and install the file. */
+	/* Bump the usage count and install the file. The resulting value of
+	 * "error" is ignored here since we only need to take action when
+	 * the file is a socket and testing "sock" for NULL is sufficient.
+	 */
 	sock = sock_from_file(file, &error);
 	if (sock) {
 		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
 		sock_update_classid(&sock->sk->sk_cgrp_data);
 	}
 	fd_install(new_fd, get_file(file));
-	return 0;
+	return new_fd;
 }
 
 static int scm_max_fds(struct msghdr *msg)
@@ -337,7 +350,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 
 	for (i = 0; i < fdmax; i++) {
 		err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
-		if (err)
+		if (err < 0)
 			break;
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] Use __scm_install_fd() more widely
  2020-06-10  4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
  2020-06-10  4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
  2020-06-10  4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
@ 2020-06-10  9:47 ` Christian Brauner
  2020-06-10 14:52   ` Kees Cook
  2020-06-10 17:03   ` Kees Cook
  2 siblings, 2 replies; 9+ messages in thread
From: Christian Brauner @ 2020-06-10  9:47 UTC (permalink / raw)
  To: Kees Cook
  Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
	Sargun Dhillon, Jakub Kicinski, netdev, linux-kernel

On Tue, Jun 09, 2020 at 09:52:12PM -0700, Kees Cook wrote:
> Hi,
> 
> This extends the recent work hch did for scm_detach_fds(), and updates
> the compat path as well, fixing bugs in the process. Additionally,
> an effectively incomplete and open-coded __scm_install_fd() is fixed
> in pidfd_getfd().

Since __scm_detach_fds() becomes something that is available outside of
net/* should we provide a static inline wrapper under a different name? The
"socket-level control message" prefix seems a bit odd in pidfd_getfd()
and - once we make use of it there - seccomp.

I'd suggest we do:

static inline int fd_install_received(struct file *file, unsigned int flags)
{
	return __scm_install_fd(file, NULL, flags);
}

which can be called in pidfd_getfd() and once we have other callers that
want the additional put_user() (e.g. seccomp_ in there we simply add:

static inline fd_install_user(struct file *file, unsigned int flags, int __user *ufd)
{
	return __scm_install_fd(file, ufd, flags);
}

and seems the wrappers both could happily live in the fs part of the world?

Christian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] Use __scm_install_fd() more widely
  2020-06-10  9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
@ 2020-06-10 14:52   ` Kees Cook
  2020-06-10 17:03   ` Kees Cook
  1 sibling, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10 14:52 UTC (permalink / raw)
  To: Christian Brauner
  Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
	Sargun Dhillon, Jakub Kicinski, netdev, linux-kernel

On Wed, Jun 10, 2020 at 11:47:35AM +0200, Christian Brauner wrote:
> On Tue, Jun 09, 2020 at 09:52:12PM -0700, Kees Cook wrote:
> > Hi,
> > 
> > This extends the recent work hch did for scm_detach_fds(), and updates
> > the compat path as well, fixing bugs in the process. Additionally,
> > an effectively incomplete and open-coded __scm_install_fd() is fixed
> > in pidfd_getfd().
> 
> Since __scm_detach_fds() becomes something that is available outside of
> net/* should we provide a static inline wrapper under a different name? The
> "socket-level control message" prefix seems a bit odd in pidfd_getfd()
> and - once we make use of it there - seccomp.
> 
> I'd suggest we do:
> 
> static inline int fd_install_received(struct file *file, unsigned int flags)
> {
> 	return __scm_install_fd(file, NULL, flags);
> }
> 
> which can be called in pidfd_getfd() and once we have other callers that
> want the additional put_user() (e.g. seccomp_ in there we simply add:
> 
> static inline fd_install_user(struct file *file, unsigned int flags, int __user *ufd)
> {
> 	return __scm_install_fd(file, ufd, flags);
> }
> 
> and seems the wrappers both could happily live in the fs part of the world?

Yeah, this seems good. I also note that randconfigs are kicking back my
series as broken when CONFIG_NET=n (oops), so this needs some refactoring
before patch 2.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd()
  2020-06-10  4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
@ 2020-06-10 16:45   ` Sargun Dhillon
  0 siblings, 0 replies; 9+ messages in thread
From: Sargun Dhillon @ 2020-06-10 16:45 UTC (permalink / raw)
  To: Kees Cook
  Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
	Jakub Kicinski, netdev, linux-kernel

On Tue, Jun 09, 2020 at 09:52:14PM -0700, Kees Cook wrote:
> The sock counting (sock_update_netprioidx() and sock_update_classid())
> was missing from this implementation of fd installation, compared to
> SCM_RIGHTS. Use the new scm helper to get the work done, after adjusting
> it to return the installed fd and accept a NULL user pointer.
> 
> Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> AFAICT, the following patches are needed for back-porting this to stable:
> 
> 0462b6bdb644 ("net: add a CMSG_USER_DATA macro")
> 2618d530dd8b ("net/scm: cleanup scm_detach_fds")
> 1f466e1f15cf ("net: cleanly handle kernel vs user buffers for ->msg_control")
> 6e8a4f9dda38 ("net: ignore sock_from_file errors in __scm_install_fd")
> ---
>  kernel/pid.c   | 12 ++----------
>  net/compat.c   |  2 +-
>  net/core/scm.c | 27 ++++++++++++++++++++-------
>  3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f1496b757162..a7ce4ba898d3 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -42,6 +42,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/sched/task.h>
>  #include <linux/idr.h>
> +#include <net/scm.h>
>  
>  struct pid init_struct_pid = {
>  	.count		= REFCOUNT_INIT(1),
> @@ -635,18 +636,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 = __scm_install_fd(file, NULL, O_CLOEXEC);
>  	if (ret < 0)
>  		fput(file);
> -	else
> -		fd_install(ret, file);
> -
>  	return ret;
>  }
>  
> diff --git a/net/compat.c b/net/compat.c
> index 117f1869bf3b..f8575555b6d7 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -299,7 +299,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
>  
>  	for (i = 0; i < fdmax; i++) {
>  		err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> -		if (err)
> +		if (err < 0)
>  			break;
>  	}
>  
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 86d96152646f..e80648fb4da7 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -280,6 +280,14 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
>  }
>  EXPORT_SYMBOL(put_cmsg_scm_timestamping);
>  
> +/**
> + * __scm_install_fd() - Install received file into file descriptor table
Any reason not to rename this remote_install_* or similar, and move it to fs/?
> + *
> + * Installs a received file into the file descriptor table, with appropriate
> + * checks and count updates.
> + *
> + * Returns fd installed or -ve on error.
> + */
>  int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
>  {
>  	struct socket *sock;
> @@ -294,20 +302,25 @@ int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
>  	if (new_fd < 0)
>  		return new_fd;
>  
> -	error = put_user(new_fd, ufd);
> -	if (error) {
> -		put_unused_fd(new_fd);
> -		return error;
> +	if (ufd) {
See my comment elsewhere about not being able to use NULL here.
> +		error = put_user(new_fd, ufd);
> +		if (error) {
> +			put_unused_fd(new_fd);
> +			return error;
> +		}
>  	}
>  
> -	/* Bump the usage count and install the file. */
> +	/* Bump the usage count and install the file. The resulting value of
> +	 * "error" is ignored here since we only need to take action when
> +	 * the file is a socket and testing "sock" for NULL is sufficient.
> +	 */
>  	sock = sock_from_file(file, &error);
>  	if (sock) {
>  		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
>  		sock_update_classid(&sock->sk->sk_cgrp_data);
>  	}
>  	fd_install(new_fd, get_file(file));
> -	return 0;
> +	return new_fd;
>  }
>  
>  static int scm_max_fds(struct msghdr *msg)
> @@ -337,7 +350,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>  
>  	for (i = 0; i < fdmax; i++) {
>  		err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> -		if (err)
> +		if (err < 0)
>  			break;
>  	}
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] Use __scm_install_fd() more widely
  2020-06-10  9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
  2020-06-10 14:52   ` Kees Cook
@ 2020-06-10 17:03   ` Kees Cook
  2020-06-10 18:38     ` Jakub Kicinski
  1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2020-06-10 17:03 UTC (permalink / raw)
  To: Christian Brauner
  Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
	Sargun Dhillon, Jakub Kicinski, netdev, linux-kernel

On Wed, Jun 10, 2020 at 11:47:35AM +0200, Christian Brauner wrote:
> On Tue, Jun 09, 2020 at 09:52:12PM -0700, Kees Cook wrote:
> > Hi,
> > 
> > This extends the recent work hch did for scm_detach_fds(), and updates
> > the compat path as well, fixing bugs in the process. Additionally,
> > an effectively incomplete and open-coded __scm_install_fd() is fixed
> > in pidfd_getfd().
> 
> Since __scm_detach_fds() becomes something that is available outside of
> net/* should we provide a static inline wrapper under a different name? The
> "socket-level control message" prefix seems a bit odd in pidfd_getfd()
> and - once we make use of it there - seccomp.
> 
> I'd suggest we do:
> 
> static inline int fd_install_received(struct file *file, unsigned int flags)
> {
> 	return __scm_install_fd(file, NULL, flags);
> }
> 
> which can be called in pidfd_getfd() and once we have other callers that
> want the additional put_user() (e.g. seccomp_ in there we simply add:
> 
> static inline fd_install_user(struct file *file, unsigned int flags, int __user *ufd)
> {
> 	return __scm_install_fd(file, ufd, flags);
> }
> 
> and seems the wrappers both could happily live in the fs part of the world?

I combined your and Sargun's suggestions. (It can't live in any more
net/core/scm.c in the case of CONFIG_NET=n, but the wrappers make the
changes much nicer looking.)

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=devel/seccomp/addfd/v3.2

If 0-day doesn't kick anything back on this tree, I'll resend the
series...

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] Use __scm_install_fd() more widely
  2020-06-10 17:03   ` Kees Cook
@ 2020-06-10 18:38     ` Jakub Kicinski
  2020-06-10 19:45       ` Kees Cook
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-06-10 18:38 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christian Brauner, David S. Miller, Christoph Hellwig,
	Christian Brauner, Sargun Dhillon, netdev, linux-kernel

On Wed, 10 Jun 2020 10:03:03 -0700 Kees Cook wrote:
> If 0-day doesn't kick anything back on this tree, I'll resend the
> series...

Well, 0-day may find more, but I can already tell you that patch 1 has
a checkpatch error:

ERROR: "(foo*)" should be "(foo *)"
#149: FILE: net/core/scm.c:323:
+		(__force struct cmsghdr __user*)msg->msg_control;

total: 1 errors, 0 warnings, 0 checks, 131 lines checked

And patch 2 makes W=1 builds unhappy:

net/core/scm.c:292: warning: Function parameter or member 'file' not described in '__scm_install_fd'
net/core/scm.c:292: warning: Function parameter or member 'ufd' not described in '__scm_install_fd'
net/core/scm.c:292: warning: Function parameter or member 'o_flags' not described in '__scm_install_fd'

:)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] Use __scm_install_fd() more widely
  2020-06-10 18:38     ` Jakub Kicinski
@ 2020-06-10 19:45       ` Kees Cook
  0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10 19:45 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Christian Brauner, David S. Miller, Christoph Hellwig,
	Christian Brauner, Sargun Dhillon, netdev, linux-kernel

On Wed, Jun 10, 2020 at 11:38:00AM -0700, Jakub Kicinski wrote:
> On Wed, 10 Jun 2020 10:03:03 -0700 Kees Cook wrote:
> > If 0-day doesn't kick anything back on this tree, I'll resend the
> > series...
> 
> Well, 0-day may find more, but I can already tell you that patch 1 has
> a checkpatch error:
> 
> ERROR: "(foo*)" should be "(foo *)"
> #149: FILE: net/core/scm.c:323:
> +		(__force struct cmsghdr __user*)msg->msg_control;
> 
> total: 1 errors, 0 warnings, 0 checks, 131 lines checked
> 
> And patch 2 makes W=1 builds unhappy:
> 
> net/core/scm.c:292: warning: Function parameter or member 'file' not described in '__scm_install_fd'
> net/core/scm.c:292: warning: Function parameter or member 'ufd' not described in '__scm_install_fd'
> net/core/scm.c:292: warning: Function parameter or member 'o_flags' not described in '__scm_install_fd'

Yeah, there are a few more problems too. I'll get it nailed down
hopefully this week and send a v2.

Thanks for looking at it!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-06-10 19:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10  4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
2020-06-10  4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
2020-06-10  4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
2020-06-10 16:45   ` Sargun Dhillon
2020-06-10  9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
2020-06-10 14:52   ` Kees Cook
2020-06-10 17:03   ` Kees Cook
2020-06-10 18:38     ` Jakub Kicinski
2020-06-10 19:45       ` Kees Cook

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