lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH V2] ipc/mqueue.c: change __do_notify() to bypass check_kill_permission()
Oleg Nesterov <oleg@redhat.com> writes:

> Commit cc731525f26a ("signal: Remove kernel interal si_code magic")
> changed the value of SI_FROMUSER(SI_MESGQ), this means that mq_notify()
> no longer works if the sender doesn't have rights to send a signal.
>
> Change __do_notify() to use do_send_sig_info() instead of kill_pid_info()
> to avoid check_kill_permission().
>
> This needs the additional notify.sigev_signo != 0 check, shouldn't we
> change do_mq_notify() to deny sigev_signo == 0 ?
>
> Test-case:
>
> #include <signal.h>
> #include <mqueue.h>
> #include <unistd.h>
> #include <sys/wait.h>
> #include <assert.h>
>
> static int notified;
>
> static void sigh(int sig)
> {
> notified = 1;
> }
>
> int main(void)
> {
> signal(SIGIO, sigh);
>
> int fd = mq_open("/mq", O_RDWR|O_CREAT, 0666, NULL);
> assert(fd >= 0);
>
> struct sigevent se = {
> .sigev_notify = SIGEV_SIGNAL,
> .sigev_signo = SIGIO,
> };
> assert(mq_notify(fd, &se) == 0);
>
> if (!fork()) {
> assert(setuid(1) == 0);
> mq_send(fd, "",1,0);
> return 0;
> }
>
> wait(NULL);
> mq_unlink("/mq");
> assert(notified);
> return 0;
> }
>
> Reported-by: Yoji <yoji.fujihar.min@gmail.com>
> Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> ipc/mqueue.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 49a05ba3000d..63b164932ffd 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -774,28 +774,40 @@ static void __do_notify(struct mqueue_inode_info *info)
> * synchronously. */
> if (info->notify_owner &&
> info->attr.mq_curmsgs == 1) {
> - struct kernel_siginfo sig_i;
> switch (info->notify.sigev_notify) {
> case SIGEV_NONE:
> break;
> - case SIGEV_SIGNAL:
> - /* sends signal */
> + case SIGEV_SIGNAL: {
> + struct kernel_siginfo sig_i;
> + struct task_struct *task;
> +
> + /* do_mq_notify() accepts sigev_signo == 0, why?? */
> + if (!info->notify.sigev_signo)
> + break;
>
> clear_siginfo(&sig_i);
> sig_i.si_signo = info->notify.sigev_signo;
> sig_i.si_errno = 0;
> sig_i.si_code = SI_MESGQ;
> sig_i.si_value = info->notify.sigev_value;
> - /* map current pid/uid into info->owner's namespaces */
> rcu_read_lock();
> + /* map current pid/uid into info->owner's namespaces */
> sig_i.si_pid = task_tgid_nr_ns(current,
> ns_of_pid(info->notify_owner));
> - sig_i.si_uid = from_kuid_munged(info->notify_user_ns, current_uid());
> + sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
> + current_uid());
> + /*
> + * We can't use kill_pid_info(), this signal should
> + * bypass check_kill_permission(). It is from kernel
> + * but si_fromuser() can't know this.
> + */
> + task = pid_task(info->notify_owner, PIDTYPE_PID);
^^^^^^^^^^^^
Minor nit: If we are doing the task lookup ourselves that can and
should be PIDTYPE_TGID. Because the code captures the TGID
itself none of the very valid backwards compatibility
reasons for using PIDTYPE_PID come into play.

> + if (task)
> + do_send_sig_info(info->notify.sigev_signo,
> + &sig_i, task, PIDTYPE_TGID);
> rcu_read_unlock();
> -
> - kill_pid_info(info->notify.sigev_signo,
> - &sig_i, info->notify_owner);
> break;
> + }
> case SIGEV_THREAD:
> set_cookie(info->notify_cookie, NOTIFY_WOKENUP);
> netlink_sendskb(info->notify_sock, info->notify_cookie);
Eric

\
 
 \ /
  Last update: 2020-03-26 13:57    [W:0.088 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site