Skip to content

Commit a318f12

Browse files
keestorvalds
authored andcommitted
ipc/mqueue.c: only perform resource calculation if user valid
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>
1 parent 6b15f67 commit a318f12

File tree

1 file changed

+10
-9
lines changed

1 file changed

+10
-9
lines changed

ipc/mqueue.c

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,6 @@ static void mqueue_evict_inode(struct inode *inode)
438438
{
439439
struct mqueue_inode_info *info;
440440
struct user_struct *user;
441-
unsigned long mq_bytes, mq_treesize;
442441
struct ipc_namespace *ipc_ns;
443442
struct msg_msg *msg, *nmsg;
444443
LIST_HEAD(tmp_msg);
@@ -461,16 +460,18 @@ static void mqueue_evict_inode(struct inode *inode)
461460
free_msg(msg);
462461
}
463462

464-
/* Total amount of bytes accounted for the mqueue */
465-
mq_treesize = info->attr.mq_maxmsg * sizeof(struct msg_msg) +
466-
min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) *
467-
sizeof(struct posix_msg_tree_node);
468-
469-
mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
470-
info->attr.mq_msgsize);
471-
472463
user = info->user;
473464
if (user) {
465+
unsigned long mq_bytes, mq_treesize;
466+
467+
/* Total amount of bytes accounted for the mqueue */
468+
mq_treesize = info->attr.mq_maxmsg * sizeof(struct msg_msg) +
469+
min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) *
470+
sizeof(struct posix_msg_tree_node);
471+
472+
mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
473+
info->attr.mq_msgsize);
474+
474475
spin_lock(&mq_lock);
475476
user->mq_bytes -= mq_bytes;
476477
/*

0 commit comments

Comments
 (0)