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

[Eric W. Biederman]

ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:

> Oleg Nesterov <oleg@xxxxxxxxxx> 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.

Scratch the fctnl(F_SETOWN,...) case for sending SIGIO.  While
frequently that applies to sending to yourself it isn't required and
that path does have a permission check.  So that whole case is clearly
something different.

That still leaves us with at least 3 very similar cases in the kernel.

Eric


> 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@xxxxxxxxx>
>> Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
>> Cc: stable <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
>> ---
>>  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);