| From be1dca967899b6f6c4dd834dae5e29ba6548f449 Mon Sep 17 00:00:00 2001 |
| From: Kees Cook <keescook@chromium.org> |
| Date: Tue, 16 Jul 2019 16:30:21 -0700 |
| Subject: 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> |
| --- |
| ipc/mqueue.c | 19 ++++++++++--------- |
| 1 file changed, 10 insertions(+), 9 deletions(-) |
| |
| diff --git a/ipc/mqueue.c b/ipc/mqueue.c |
| index d5491a8807515..3f7dc5f341f7e 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; |
| /* |
| -- |
| 2.20.1 |
| |