diff mbox series

[v7,3/9] net/scm: Regularize compat handling of scm_detach_fds()

Message ID 20200709182642.1773477-4-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series Add seccomp notifier ioctl that enables adding fds | expand

Commit Message

Kees Cook July 9, 2020, 6:26 p.m. UTC 
Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
scm_detach_fds") into the compat code.

Replace open-coded __receive_sock() with a call to the helper.

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

Correct the int "flags" argument to unsigned int to match fd_install()
and similar APIs.

Regularize any remaining differences, including a whitespace issue,
a checkpatch warning, and add 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.

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/net/scm.h |  1 +
 net/compat.c      | 56 +++++++++++++++++++++--------------------------
 net/core/scm.c    | 27 ++++++++++-------------
 3 files changed, 37 insertions(+), 47 deletions(-)

Comments

John Stultz Aug. 7, 2020, 8:29 p.m. UTC | #1 
On Thu, Jul 9, 2020 at 11:28 AM Kees Cook <keescook@chromium.org> wrote:
>
> Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
> scm_detach_fds") into the compat code.
>
> Replace open-coded __receive_sock() with a call to the helper.
>
> Move 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.
>
> Correct the int "flags" argument to unsigned int to match fd_install()
> and similar APIs.
>
> Regularize any remaining differences, including a whitespace issue,
> a checkpatch warning, and add 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.
>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Hey Kees,
  So during the merge window (while chasing a few other regressions),
I noticed occasionally my Dragonboard 845c running AOSP having trouble
with the web browser crashing or other apps hanging, and I've bisected
the issue down to this change.

Unfortunately it doesn't revert cleanly so I can't validate reverting
it sorts things against linus/HEAD.  Anyway, I wanted to check and see
if you had any other reports of similar or any ideas what might be
going wrong?

thanks
-john
Kees Cook Aug. 7, 2020, 10:18 p.m. UTC | #2 
On Fri, Aug 07, 2020 at 01:29:24PM -0700, John Stultz wrote:
> On Thu, Jul 9, 2020 at 11:28 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
> > scm_detach_fds") into the compat code.
> >
> > Replace open-coded __receive_sock() with a call to the helper.
> >
> > Move 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.
> >
> > Correct the int "flags" argument to unsigned int to match fd_install()
> > and similar APIs.
> >
> > Regularize any remaining differences, including a whitespace issue,
> > a checkpatch warning, and add 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.
> >
> > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> Hey Kees,
>   So during the merge window (while chasing a few other regressions),
> I noticed occasionally my Dragonboard 845c running AOSP having trouble
> with the web browser crashing or other apps hanging, and I've bisected
> the issue down to this change.
> 
> Unfortunately it doesn't revert cleanly so I can't validate reverting
> it sorts things against linus/HEAD.  Anyway, I wanted to check and see
> if you had any other reports of similar or any ideas what might be
> going wrong?

Hi; Yes, sorry for the trouble. I had a typo in a refactor of
SCM_RIGHTS. I suspect it'll be fixed by this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1fa2c0a0c814fbae0eb3e79a510765225570d043

Can you verify Linus's latest tree works for you? If not, there might be
something else hiding in the corners...

Thanks!
John Stultz Aug. 8, 2020, 12:02 a.m. UTC | #3 
On Fri, Aug 7, 2020 at 3:18 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Aug 07, 2020 at 01:29:24PM -0700, John Stultz wrote:
> > On Thu, Jul 9, 2020 at 11:28 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
> > > scm_detach_fds") into the compat code.
> > >
> > > Replace open-coded __receive_sock() with a call to the helper.
> > >
> > > Move 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.
> > >
> > > Correct the int "flags" argument to unsigned int to match fd_install()
> > > and similar APIs.
> > >
> > > Regularize any remaining differences, including a whitespace issue,
> > > a checkpatch warning, and add 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.
> > >
> > > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> >
> > Hey Kees,
> >   So during the merge window (while chasing a few other regressions),
> > I noticed occasionally my Dragonboard 845c running AOSP having trouble
> > with the web browser crashing or other apps hanging, and I've bisected
> > the issue down to this change.
> >
> > Unfortunately it doesn't revert cleanly so I can't validate reverting
> > it sorts things against linus/HEAD.  Anyway, I wanted to check and see
> > if you had any other reports of similar or any ideas what might be
> > going wrong?
>
> Hi; Yes, sorry for the trouble. I had a typo in a refactor of
> SCM_RIGHTS. I suspect it'll be fixed by this:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1fa2c0a0c814fbae0eb3e79a510765225570d043
>
> Can you verify Linus's latest tree works for you? If not, there might be
> something else hiding in the corners...

Thanks so much! Yes, I just updated to Linus' latest and the issue has
disappeared!

thanks again!
-john
Kees Cook Aug. 8, 2020, 7:17 a.m. UTC | #4 
On Fri, Aug 07, 2020 at 05:02:15PM -0700, John Stultz wrote:
> On Fri, Aug 7, 2020 at 3:18 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Aug 07, 2020 at 01:29:24PM -0700, John Stultz wrote:
> > > On Thu, Jul 9, 2020 at 11:28 AM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
> > > > scm_detach_fds") into the compat code.
> > > >
> > > > Replace open-coded __receive_sock() with a call to the helper.
> > > >
> > > > Move 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.
> > > >
> > > > Correct the int "flags" argument to unsigned int to match fd_install()
> > > > and similar APIs.
> > > >
> > > > Regularize any remaining differences, including a whitespace issue,
> > > > a checkpatch warning, and add 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.
> > > >
> > > > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > >
> > > Hey Kees,
> > >   So during the merge window (while chasing a few other regressions),
> > > I noticed occasionally my Dragonboard 845c running AOSP having trouble
> > > with the web browser crashing or other apps hanging, and I've bisected
> > > the issue down to this change.
> > >
> > > Unfortunately it doesn't revert cleanly so I can't validate reverting
> > > it sorts things against linus/HEAD.  Anyway, I wanted to check and see
> > > if you had any other reports of similar or any ideas what might be
> > > going wrong?
> >
> > Hi; Yes, sorry for the trouble. I had a typo in a refactor of
> > SCM_RIGHTS. I suspect it'll be fixed by this:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1fa2c0a0c814fbae0eb3e79a510765225570d043
> >
> > Can you verify Linus's latest tree works for you? If not, there might be
> > something else hiding in the corners...
> 
> Thanks so much! Yes, I just updated to Linus' latest and the issue has
> disappeared!
> 
> thanks again!

Whew; sorry again and thanks for testing! :)
diff mbox series

Patch

diff --git a/include/net/scm.h b/include/net/scm.h
index 1ce365f4c256..581a94d6c613 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, unsigned 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 2937b816107d..27d477fdcaa0 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -281,40 +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;
+	unsigned 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. */
-		__receive_sock(fp[i]);
-		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);
@@ -322,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..44f03213dcab 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -280,9 +280,8 @@  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, unsigned int o_flags)
 {
-	struct socket *sock;
 	int new_fd;
 	int error;
 
@@ -300,12 +299,8 @@  static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
 		return error;
 	}
 
-	/* Bump the usage count and install the file. */
-	sock = sock_from_file(file, &error);
-	if (sock) {
-		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
-		sock_update_classid(&sock->sk->sk_cgrp_data);
-	}
+	/* Bump the sock usage counts, if any. */
+	__receive_sock(file);
 	fd_install(new_fd, get_file(file));
 	return 0;
 }
@@ -319,29 +314,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;
-	int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
+	struct cmsghdr __user *cm =
+		(__force struct cmsghdr __user *)msg->msg_control;
+	unsigned 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);