Re: [PATCH v7 3/9] net/scm: Regularize compat handling of scm_detach_fds()
On Fri, Aug 07, 2020 at 05:02:15PM -0700, John Stultz wrote:
> On Fri, Aug 7, 2020 at 3:18 PM Kees Cook 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 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
> > > > Signed-off-by: Kees Cook
> > > > ---
> > >
> > > 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! :)
--
Kees Cook
Re: [PATCH v7 3/9] net/scm: Regularize compat handling of scm_detach_fds()
On Fri, Aug 7, 2020 at 3:18 PM Kees Cook 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 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
> > > Signed-off-by: Kees Cook
> > > ---
> >
> > 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
Re: [PATCH v7 3/9] net/scm: Regularize compat handling of scm_detach_fds()
On Fri, Aug 07, 2020 at 01:29:24PM -0700, John Stultz wrote:
> On Thu, Jul 9, 2020 at 11:28 AM Kees Cook 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
> > Signed-off-by: Kees Cook
> > ---
>
> 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!
--
Kees Cook
Re: [PATCH v7 3/9] net/scm: Regularize compat handling of scm_detach_fds()
On Thu, Jul 9, 2020 at 11:28 AM Kees Cook 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
> Signed-off-by: Kees Cook
> ---
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
[PATCH v7 3/9] net/scm: Regularize compat handling of scm_detach_fds()
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
Signed-off-by: Kees Cook
---
include/net/scm.h | 1 +
net/compat.c | 56 +--
net/core/scm.c| 27 ++-
3 files changed, 37 insertions(+), 47 deletions(-)
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
