linux-kernel - Re: [PATCH] ipc/mqueue.c: change __do_notify() to bypass check_kill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87lfnsh3tm.fsf@x220.int.ebiederm.org>
Date:   Sun, 22 Mar 2020 09:17:09 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Manfred Spraul <manfred@...orfullife.com>,
        Markus Elfring <elfring@...rs.sourceforge.net>,
        Yoji <yoji.fujihar.min@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipc/mqueue.c: change __do_notify() to bypass check_kill_permission()

Oleg Nesterov <oleg@...hat.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().

I totally see why you are doing this.  To avoid the permission check,
and since this process requested the signal it makes sense to bypass the
permission checks.  The code needs to make certain that this signal is
canceled or otherwise won't be sent after an exec.

That said I don't like it.  I would really like to remove the signal
sending interfaces that take a task_struct.

Looking at the code I currently see several places where we have this
kind of semantic (sending a requested signal to a process from the
context of another process): do_notify_parent, pdeath_signal, f_setown,
and mq_notify.

When 4 different pieces of code are doing effectively the same thing and
have very similar if not the exact same concerns and they are all doing
things differently then we have a maintenance problem.

Especially with the concerns about being able to send a signal after
exec, and cause havoc.

Oleg is there any chance you can see if you can find a common helper
or a common idiom that all three cases can and should use?  Espeically
with concerns about being able to send signals to a suid process that
would normally fail I think there is an issue here.

At the very least can you add a big fat comment about the semantics
that userspace expects in this case?

> This needs the additional notify.sigev_signo != 0 check, shouldn't we
> change do_mq_notify() to deny sigev_signo == 0 ?

I wonder if the author of the code simply did not realize that
valid_signal allows 0.  As this is a posix interface we should be able
to check the posix spec and see if it gives any useful guidance about
signal 0.

Eric

> Reported-by: Yoji <yoji.fujihar.min@...il.com>
> Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
> Cc: stable <stable@...r.kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  ipc/mqueue.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 49a05ba3000d..3145fae162c1 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -775,12 +775,15 @@ static void __do_notify(struct mqueue_inode_info *info)
>  	if (info->notify_owner &&
>  	    info->attr.mq_curmsgs == 1) {
>  		struct kernel_siginfo sig_i;
> +		struct task_struct *task;
>  		switch (info->notify.sigev_notify) {
>  		case SIGEV_NONE:
>  			break;
>  		case SIGEV_SIGNAL:
> +			/* do_mq_notify() accepts sigev_signo == 0, why?? */
> +			if (!info->notify.sigev_signo)
> +				break;
>  			/* sends signal */
> -
>  			clear_siginfo(&sig_i);
>  			sig_i.si_signo = info->notify.sigev_signo;
>  			sig_i.si_errno = 0;
> @@ -790,11 +793,13 @@ static void __do_notify(struct mqueue_inode_info *info)
>  			rcu_read_lock();
>  			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());
> +			task = pid_task(info->notify_owner, PIDTYPE_PID);
> +			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);