Re: [PATCH v3 1/4] IPC: Added two new system call mq_recvmmsg() and mq_sendmmsg()

Hi Mathura_Kumar,

I haven't had a chance to look closely at the patches yet. Here is the
sashiko-like report.
Please review these comments for relevance.

> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 02c88cbb4582..188ac8547e5a 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -739,7 +743,13 @@ asmlinkage long sys_sysinfo(struct sysinfo __user *info);
>  asmlinkage long sys_mq_open(const char __user *name, int oflag, umode_t mode, struct mq_attr __user *attr);
>  asmlinkage long sys_mq_unlink(const char __user *name);
>  asmlinkage long sys_mq_timedsend(mqd_t mqdes, const char __user *msg_ptr, size_t msg_len, unsigned int msg_prio, const struct __kernel_timespec __user *abs_timeout);
> +asmlinkage long sys_mq_sendmmsg(mqd_t mqdes, struct mq_mmsg_attrs __user *attrs,
> +    unsigned int attrs_len, unsigned int flags, unsigned long start_index,
> +    const struct __kernel_timespec __user *abs_timeout);
>  asmlinkage long sys_mq_timedreceive(mqd_t mqdes, char __user *msg_ptr, size_t msg_len, unsigned int __user *msg_prio, const struct __kernel_timespec __user *abs_timeout);

There seems to be a mismatch between this declaration and the actual
implementation of sys_mq_sendmmsg in ipc/mqueue.c.

The declaration above includes an extra "flags" argument (6 arguments total),
but the implementation uses SYSCALL_DEFINE5 (5 arguments total, without flags):

SYSCALL_DEFINE5(mq_sendmmsg, mqd_t, mqdes, struct mq_mmsg_attrs __user *, attrs,
               unsigned int, attrs_len, unsigned long, start_idx,
               const struct __kernel_timespec __user *, u_abs_timeout)

This mismatch will cause argument shifting when the syscall is invoked,
leading to incorrect parameter interpretation (e.g. start_index being treated
as the timeout pointer) and likely crashes or EFAULT.

Should the declaration be updated to match the 5-argument implementation?
The compat declaration in include/linux/compat.h also seems to have the same
issue.

[ ... ]

> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 4798b375972b..99c0aa3d9c9d 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -1380,6 +1380,12 @@ static ssize_t do_mq_recvmsg2(mqd_t mqdes, struct mq_msg_attrs *args, unsigned i
> k_msg_buffer = alloc_msg(k_m_ts);
>
> if (!k_msg_buffer)
> return -ENOMEM;
> ret = security_msg_msg_alloc(k_msg_buffer);
> if (ret)
> return ret;

If security_msg_msg_alloc() fails, it returns an error code, but it doesn't
seem to free the recently allocated k_msg_buffer.

Does this leak the k_msg_buffer? Should it call free_msg(k_msg_buffer) before
returning?

[ ... ]

> diff --git a/ipc/msgutil.c b/ipc/msgutil.c
> index e28f0cecb2ec..f137fb2046a7 100644
> --- a/ipc/msgutil.c
> +++ b/ipc/msgutil.c
> @@ -122,39 +122,37 @@ struct msg_msg *load_msg(const void __user *src, size_t len)
>
> +struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst, size_t len)
>  {
> - struct msg_msgseg *dst_pseg, *src_pseg;
> - size_t len = src->m_ts;
> - size_t alen;
> + struct msg_msgseg *src_seg, *dst_seg;
> + size_t remaining, chunk;
>
> - if (src->m_ts > dst->m_ts)
> + if (len > src->m_ts)
>  return ERR_PTR(-EINVAL);
>
> - alen = min(len, DATALEN_MSG);
> - memcpy(dst + 1, src + 1, alen);
> + chunk = min(len, DATALEN_MSG);
>
> - for (dst_pseg = dst->next, src_pseg = src->next;
> -      src_pseg != NULL;
> -      dst_pseg = dst_pseg->next, src_pseg = src_pseg->next) {
> + memcpy(dst + 1, src + 1, chunk);
> + remaining = len - chunk;
> + src_seg = src->next;
> + dst_seg = dst->next;
>
> - len -= alen;
> - alen = min(len, DATALEN_SEG);
> - memcpy(dst_pseg + 1, src_pseg + 1, alen);
> + while (remaining > 0 && src_seg && dst_seg) {
> + chunk = min(remaining, DATALEN_SEG);
> + memcpy(dst_seg + 1, src_seg + 1, chunk);
> + remaining -= chunk;
> + src_seg = src_seg->next;
> + dst_seg = dst_seg->next;
>  }
> -
> + if (remaining != 0)
> + return ERR_PTR(-EINVAL);
>  dst->m_type = src->m_type;
> - dst->m_ts = src->m_ts;
> -
> + dst->m_ts = src->m_ts;
>  return dst;
>  }

In the updated copy_msg() helper, the "len" parameter specifies how many
bytes to copy, and it is validated to be less than or equal to src->m_ts.

If copy_msg() is called with "len" less than src->m_ts, it will copy only
"len" bytes, but the destination message size (dst->m_ts) is still set to
src->m_ts.

If the destination buffer was allocated based on "len" (which is smaller
than src->m_ts), setting dst->m_ts to a larger value might lead to out-of-bounds
reads when dst->m_ts is used later to copy the message to user space.

Should dst->m_ts be set to "len" instead of "src->m_ts"?