Re: [PATCH V2] ipc/mqueue.c: change __do_notify() to bypass check_kill_permission()

[ebiederm@xmission.com (Eric W. Biederman)]

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