[PATCH] ipc/mqueue: Only perform resource calculation if user valid

[PATCH] ipc/mqueue: Only perform resource calculation if user valid

[Kees Cook]

Andreas Christoforou reported:

UBSAN: Undefined behaviour in ipc/mqueue.c:414:49 signed integer overflow:
9 * 2305843009213693951 cannot be represented in type 'long int'
...
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x11b/0x1fe lib/dump_stack.c:113
 ubsan_epilogue+0xe/0x81 lib/ubsan.c:159
 handle_overflow+0x193/0x218 lib/ubsan.c:190
 mqueue_evict_inode+0x8e7/0xa10 ipc/mqueue.c:414
 evict+0x472/0x8c0 fs/inode.c:558
 iput_final fs/inode.c:1547 [inline]
 iput+0x51d/0x8c0 fs/inode.c:1573
 mqueue_get_inode+0x8eb/0x1070 ipc/mqueue.c:320
 mqueue_create_attr+0x198/0x440 ipc/mqueue.c:459
 vfs_mkobj+0x39e/0x580 fs/namei.c:2892
 prepare_open ipc/mqueue.c:731 [inline]
 do_mq_open+0x6da/0x8e0 ipc/mqueue.c:771
...

Which could be triggered by:

        struct mq_attr attr = {
                .mq_flags = 0,
                .mq_maxmsg = 9,
                .mq_msgsize = 0x1fffffffffffffff,
                .mq_curmsgs = 0,
        };

        if (mq_open("/testing", 0x40, 3, &attr) == (mqd_t) -1)
                perror("mq_open");

mqueue_get_inode() was correctly rejecting the giant mq_msgsize,
and preparing to return -EINVAL. During the cleanup, it calls
mqueue_evict_inode() which performed resource usage tracking math for
updating "user", before checking if there was a valid "user" at all
(which would indicate that the calculations would be sane). Instead,
delay this check to after seeing a valid "user".

The overflow was real, but the results went unused, so while the flaw
is harmless, it's noisy for kernel fuzzers, and the fix makes things
technically more efficient.

Reported-by: Andreas Christoforou <andreaschristofo@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 ipc/mqueue.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index c0d58f390c3b..f31e334e3635 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -389,7 +389,6 @@ static void mqueue_evict_inode(struct inode *inode)
 {
 	struct mqueue_inode_info *info;
 	struct user_struct *user;
-	unsigned long mq_bytes, mq_treesize;
 	struct ipc_namespace *ipc_ns;
 	struct msg_msg *msg;
 
@@ -406,16 +405,18 @@ static void mqueue_evict_inode(struct inode *inode)
 	kfree(info->node_cache);
 	spin_unlock(&info->lock);
 
-	/* Total amount of bytes accounted for the mqueue */
-	mq_treesize = info->attr.mq_maxmsg * sizeof(struct msg_msg) +
-		min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) *
-		sizeof(struct posix_msg_tree_node);
-
-	mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
-				  info->attr.mq_msgsize);
-
 	user = info->user;
 	if (user) {
+		unsigned long mq_bytes, mq_treesize;
+
+		/* Total amount of bytes accounted for the mqueue */
+		mq_treesize = info->attr.mq_maxmsg * sizeof(struct msg_msg) +
+			min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) *
+			sizeof(struct posix_msg_tree_node);
+
+		mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
+					  info->attr.mq_msgsize);
+
 		spin_lock(&mq_lock);
 		user->mq_bytes -= mq_bytes;
 		/*
-- 
2.17.1


-- 
Kees Cook
Pixel Security

Re: [PATCH] ipc/mqueue: Only perform resource calculation if user valid

[Al Viro]

On Fri, Aug 24, 2018 at 02:54:39PM -0700, Kees Cook wrote:

> The overflow was real, but the results went unused, so while the flaw
> is harmless, it's noisy for kernel fuzzers, and the fix makes things
> technically more efficient.

yes, yes, yes, yes and no.  Compiler is quite capable of noticing
that targets of assignments are unused outside of that if (user), so
any mentioning of "technically more efficient" is somewhat ridiculous...

Just make that something along the lines of "results of these assignments
are only used under if (user).  Moreover, in case if info->user is NULL
they might yield an integer overflow, confusing the fuzzers.  The odds
of any compiler letting nasal demons fly on that are pretty much nil,
but it's cleaner to move these under if (user) anyway."