* [PATCH v2] ipc/mqueue: Only perform resource calculation if user valid
@ 2019-06-08 5:08 Kees Cook
2019-06-08 11:56 ` Eric W. Biederman
0 siblings, 1 reply; 2+ messages in thread
From: Kees Cook @ 2019-06-08 5:08 UTC (permalink / raw)
To: Andrew Morton
Cc: Andreas Christoforou, Al Viro, Arnd Bergmann, Eric W. Biederman,
linux-kernel
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, so just fix it by moving
the calculation under the non-NULL "user" where it actually gets used.
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>
---
v2: update commit log based on Al's feedback
---
ipc/mqueue.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 216cad1ff0d0..65c351564ad0 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -438,7 +438,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);
@@ -461,16 +460,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.17.1
--
Kees Cook
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] ipc/mqueue: Only perform resource calculation if user valid
2019-06-08 5:08 [PATCH v2] ipc/mqueue: Only perform resource calculation if user valid Kees Cook
@ 2019-06-08 11:56 ` Eric W. Biederman
0 siblings, 0 replies; 2+ messages in thread
From: Eric W. Biederman @ 2019-06-08 11:56 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andreas Christoforou, Al Viro, Arnd Bergmann,
linux-kernel
Kees Cook <keescook@chromium.org> writes:
> 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, so just fix it by moving
> the calculation under the non-NULL "user" where it actually gets used.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> 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>
> ---
> v2: update commit log based on Al's feedback
> ---
> ipc/mqueue.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 216cad1ff0d0..65c351564ad0 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -438,7 +438,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);
> @@ -461,16 +460,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.17.1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-06-08 11:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-08 5:08 [PATCH v2] ipc/mqueue: Only perform resource calculation if user valid Kees Cook
2019-06-08 11:56 ` Eric W. Biederman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).