ipc/mqueue.c: only perform resource calculation if user valid [Linux 4.9.188]

This Linux kernel change "ipc/mqueue.c: only perform resource calculation if user valid" is included in the Linux 4.9.188 release. This change is authored by Kees Cook <keescook [at] chromium.org> on Tue Jul 16 16:30:21 2019 -0700. The commit for this change in Linux stable tree is 62369d5 (patch) which is from upstream commit a318f12. The same Linux upstream change may have been applied to various maintained Linux releases and you can find all Linux releases containing changes from upstream a318f12.

ipc/mqueue.c: only perform resource calculation if user valid

[ Upstream commit a318f12ed8843cfac53198390c74a565c632f417 ]

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:
    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, so just fix it by moving the
calculation under the non-NULL "user" where it actually gets used.

Link: http://lkml.kernel.org/r/201906072207.ECB65450@keescook
Signed-off-by: Kees Cook <keescook@chromium.org>
Reported-by: Andreas Christoforou <andreaschristofo@gmail.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>

There are 19 lines of Linux source code added/deleted in this change. Code changes to Linux kernel are as follows.

 ipc/mqueue.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index d5491a8..3f7dc5f 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -369,7 +369,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, *nmsg;
    LIST_HEAD(tmp_msg);
@@ -392,16 +391,18 @@ static void mqueue_evict_inode(struct inode *inode)
        free_msg(msg);
    }

-   /* 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;
        /*

Leave a Reply

Your email address will not be published. Required fields are marked *