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

[Eric W. Biederman]

Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>

>
> Reported-by: Andreas Christoforou <andreaschristofo@xxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> 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